Truncate CRD Description Fields to 50 Characters #204

ayushsatyam146 · 2024-04-23T11:43:52Z

Changes

Adding a TruncateField transformer and using it to truncate CRD description fields from the kodata/release.yaml manifest. The CRDs for Shipwright Build objects are very large, resulting in a validation error from Kubernetes when we try to create the CRDs with Manifestival.

Partially addresses #184

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Tructate descriptions in Shipwright custom resource definitions to 50 characters.

adambkaplan

Good start! Most of my comments are related to code style, though I think I did find a bug in the implementation.

Please also add unit tests for TruncateField and the transformer function it returns. This is especially important because the function uses recursion - we need to ensure this function doesn't get stuck in an infinite loop and handles a set of expected inputs.

adambkaplan · 2024-04-23T13:59:23Z

pkg/common/util.go

+// Recursively truncates the given "field" from CustomResourceDefinition to 50 characters.
+func truncateFieldRecursively(data map[string]interface{}, maxLength int, field string) {


Use Go Doc style conventions - start the sentence with the name of the function.

Suggested change

// Recursively truncates the given "field" from CustomResourceDefinition to 50 characters.

func truncateFieldRecursively(data map[string]interface{}, maxLength int, field string) {

// truncateFieldRecursively truncates the named "field" from the given data object and its sub-objects to 50 characters.

func truncateFieldRecursively(data map[string]interface{}, maxLength int, field string) {

adambkaplan · 2024-04-23T14:02:02Z

pkg/common/util.go

+			if str, ok := value.(string); ok && len(str) > maxLength {
+				data[key] = str[:maxLength]
+			}


This appears to be truncating to the maxLength - good!

adambkaplan · 2024-04-23T14:02:26Z

pkg/common/util.go

+			continue
+		}
+		if subObj, ok := value.(map[string]interface{}); ok {
+			truncateFieldRecursively(subObj, 50, field)


Why not pass maxLength here?

Suggested change

truncateFieldRecursively(subObj, 50, field)

truncateFieldRecursively(subObj, maxLength, field)

adambkaplan · 2024-04-23T14:03:30Z

pkg/common/util.go

+		if subObjs, ok := value.([]interface{}); ok {
+			for _, subObj := range subObjs {
+				if subObjMap, ok := subObj.(map[string]interface{}); ok {
+					truncateFieldRecursively(subObjMap, 50, field)


Ditto - pass maxLength here?

Suggested change

truncateFieldRecursively(subObjMap, 50, field)

truncateFieldRecursively(subObjMap, maxLength, field)

adambkaplan · 2024-04-23T14:08:26Z

pkg/common/util.go

+// truncates the given "field" from CustomResourceDefinition to 50 characters.
+func TruncateField(field string, targetLength int) manifestival.Transformer {


Since this has special behavior for CRDs and implements the Transformer interface, I recommend updating the function name to make this more clear. I also updated the length variable to maxLength so it is consistent.

Suggested change

// truncates the given "field" from CustomResourceDefinition to 50 characters.

func TruncateField(field string, targetLength int) manifestival.Transformer {

// TruncateCRDFieldTransformer returns a manifestival.Transformer that truncates the value of the given field within a CRD spec to the provided max length.

func TruncateCRDFieldTransformer(field string, maxLength int) manifestival.Transformer {

adambkaplan · 2024-04-23T14:09:01Z

pkg/common/util.go

+			return nil
+		}
+		data := u.Object
+		truncateFieldRecursively(data, targetLength, field)


Continuing from above:

Suggested change

truncateFieldRecursively(data, targetLength, field)

truncateFieldRecursively(data, maxLength, field)

apoorvajagtap · 2024-05-03T08:36:16Z

pkg/common/util_test.go

+		yaml.Unmarshal([]byte(crdYaml), &u.Object)
+		TruncateCRDFieldTransformer("description", 10)(u)


lint errors: Need to handle &/or check the return values for yaml.Unmarshal (returns err) & TruncateCRDFieldTransformer (returns manifestival.Transformer).

adambkaplan

/approve

Code changes look good - I suggested a few nit fixes that shouldn't block merge.

Need to investigate why the lint check bombed out (may re-test without the cache).

pkg/common/util_test.go

openshift-ci · 2024-05-03T14:29:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adambkaplan · 2024-05-03T14:42:54Z

/kind bug

Adding a TruncateField transformer and using it to truncate CRD description fields from the kodata/release.yaml manifest. The CRDs for Shipwright Build objects are very large, resulting in a validation error from Kubernetes when we try to create the CRDs with Manifestival.

adambkaplan · 2024-05-03T15:16:17Z

/lgtm

🎉 Thank you!

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 23, 2024

openshift-ci bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Apr 23, 2024

openshift-ci bot requested review from adambkaplan and qu1queee April 23, 2024 11:43

adambkaplan requested changes Apr 23, 2024

View reviewed changes

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 30, 2024

ayushsatyam146 requested a review from adambkaplan April 30, 2024 11:17

ayushsatyam146 force-pushed the fix-unit-tests branch from 135ec29 to 4ebe224 Compare May 3, 2024 06:07

apoorvajagtap mentioned this pull request May 3, 2024

[WIP] Resolve lint errors w.r.t operator/pull/204 apoorvajagtap/shipwright-operator#2

Closed

apoorvajagtap suggested changes May 3, 2024

View reviewed changes

ayushsatyam146 force-pushed the fix-unit-tests branch from 545c0d9 to 8f57b3b Compare May 3, 2024 12:18

adambkaplan approved these changes May 3, 2024

View reviewed changes

pkg/common/util_test.go Outdated Show resolved Hide resolved

pkg/common/util_test.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2024

adambkaplan changed the title ~~truncating description fields to max length of 50 to avoid unit test failure~~ Truncate CRD Description Fields to 50 Characters May 3, 2024

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 3, 2024

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label May 3, 2024

adambkaplan added this to the release-v0.13.0 milestone May 3, 2024

ayushsatyam146 force-pushed the fix-unit-tests branch from 0531353 to 2e8cffc Compare May 3, 2024 14:59

ayushsatyam146 force-pushed the fix-unit-tests branch from 2e8cffc to 9290711 Compare May 3, 2024 15:10

openshift-ci bot assigned adambkaplan May 3, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2024

openshift-merge-bot bot merged commit d26c155 into shipwright-io:main May 3, 2024
4 checks passed

adambkaplan mentioned this pull request May 3, 2024

[BUG] v0.12.0 does not ship an exact match of build v0.12.0 #184

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Truncate CRD Description Fields to 50 Characters #204

Truncate CRD Description Fields to 50 Characters #204

ayushsatyam146 commented Apr 23, 2024 •

edited by adambkaplan

Loading

adambkaplan left a comment

adambkaplan Apr 23, 2024

adambkaplan Apr 23, 2024

adambkaplan Apr 23, 2024

adambkaplan Apr 23, 2024

adambkaplan Apr 23, 2024

adambkaplan Apr 23, 2024

apoorvajagtap May 3, 2024

adambkaplan left a comment

openshift-ci bot commented May 3, 2024

adambkaplan commented May 3, 2024

adambkaplan commented May 3, 2024

		// Recursively truncates the given "field" from CustomResourceDefinition to 50 characters.
		func truncateFieldRecursively(data map[string]interface{}, maxLength int, field string) {

	truncateFieldRecursively(subObj, 50, field)
	truncateFieldRecursively(subObj, maxLength, field)

	truncateFieldRecursively(subObjMap, 50, field)
	truncateFieldRecursively(subObjMap, maxLength, field)

		// truncates the given "field" from CustomResourceDefinition to 50 characters.
		func TruncateField(field string, targetLength int) manifestival.Transformer {

	truncateFieldRecursively(data, targetLength, field)
	truncateFieldRecursively(data, maxLength, field)

		yaml.Unmarshal([]byte(crdYaml), &u.Object)
		TruncateCRDFieldTransformer("description", 10)(u)

Truncate CRD Description Fields to 50 Characters #204

Truncate CRD Description Fields to 50 Characters #204

Conversation

ayushsatyam146 commented Apr 23, 2024 • edited by adambkaplan Loading

Changes

Submitter Checklist

Release Notes

adambkaplan left a comment

Choose a reason for hiding this comment

adambkaplan Apr 23, 2024

Choose a reason for hiding this comment

adambkaplan Apr 23, 2024

Choose a reason for hiding this comment

adambkaplan Apr 23, 2024

Choose a reason for hiding this comment

adambkaplan Apr 23, 2024

Choose a reason for hiding this comment

adambkaplan Apr 23, 2024

Choose a reason for hiding this comment

adambkaplan Apr 23, 2024

Choose a reason for hiding this comment

apoorvajagtap May 3, 2024

Choose a reason for hiding this comment

adambkaplan left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 3, 2024

adambkaplan commented May 3, 2024

adambkaplan commented May 3, 2024

ayushsatyam146 commented Apr 23, 2024 •

edited by adambkaplan

Loading