Removing creationTimestamp under Template object #50

dperaza4dustbit · 2024-07-15T22:59:07Z

The creationTimestamp field not only pops up under TemplateTier metadata but also under template itself

alexeykazakov

Looks good to me but it looks like there is a room for improvements in the unit tests.

dperaza4dustbit · 2024-07-15T23:33:39Z

Will try to figure out if I spot the issue with unit test

fbm3307

Apart from splitting the functions, if possible as a part of this PR we can also have a unit test to check these, they are currently not being tested, i am unsure of the reason for it

pkg/cmd/generate/util.go

dperaza4dustbit · 2024-07-18T17:40:30Z

pkg/cmd/generate/util.go

@@ -159,6 +159,7 @@ const header = `# --------------------------------------------------------------
 func writeFile(filePath string, content []byte) error {
 	// https://github.com/kubernetes/kubernetes/issues/67610
 	contentString := strings.ReplaceAll(string(content), "\n  creationTimestamp: null", "")
+	contentString = strings.ReplaceAll(contentString, "\n      creationTimestamp: null", " {}")


Need to set to empty object to fix issue with ArgoCD synch loop created by TierTemplates do to spec.template.matadata see the fix in infra deployment here: redhat-appstudio/infra-deployments#4120. The requirement is that all TierTemplates in source will start with empty object {} and then we need to remove the creationTimeStap set to null. If there is a better way to do this let me know.

let's fix this empty metadata as a separate PR as this won't work if the spec.template.metadata.name is set (for example in our e2e tests).
Let's focus on the empty creationTimestamp for now

as for the empty metadata - this is tricky one. Not sure if there is a better (generic) way to deal with this problem, but re-marshalling the object (after removing empty creationTimestamp field) and removing metadata: null should work:

rawObject := runtime.RawExtension{} err := yaml.Unmarshal([]byte(contentString), &rawObject) if err != nil { return err } rawContent, err := yaml.Marshal(rawObject) if err != nil { return err } emptyMetadata := regexp.MustCompile(`\n *metadata: null`) contentString = emptyMetadata.ReplaceAllString(string(rawContent), "")

see my new approach in utils.go, seems simpler to me

dperaza4dustbit · 2024-07-18T17:42:45Z

Working on fixing the testcases

fbm3307

This looks good,
just a minor suggestion
Either one of it can be used, whichever works

pkg/cmd/generate/util.go

MatousJobanek · 2024-07-22T12:05:50Z

pkg/cmd/generate/util.go

 	contentString := strings.ReplaceAll(string(content), "\n  creationTimestamp: null", "")
+	contentString = strings.ReplaceAll(contentString, "\n      creationTimestamp: null", " {}")


I would rather make it more generic so it replaces any empty creationTimestamp no matter at which level

Suggested change

contentString := strings.ReplaceAll(string(content), "\n creationTimestamp: null", "")

contentString = strings.ReplaceAll(contentString, "\n creationTimestamp: null", " {}")

emptyCreationTimestamp := regexp.MustCompile(`\n *creationTimestamp: null`)

contentString := emptyCreationTimestamp.ReplaceAllString(string(content), "")

Took your suggestion here

MatousJobanek · 2024-07-22T12:15:28Z

as for unit-testing - let's either improve the util_test.go so it verifies objects at any level, or add this check:

ksctl/pkg/cmd/generate/assertion_test.go

Lines 169 to 171 in d758210

    
           assert.True(t, strings.HasPrefix(string(file), header), "every file generated by cli should container a header") 
        
           assert.NotContains(t, string(file), "creationTimestamp") 
        
           assert.NotContains(t, string(file), "user: {}")

into

ksctl/pkg/cmd/generate/assertion_test.go

Lines 142 to 143 in d758210

    
           objFile, err := os.ReadFile(path) 
        
           require.NoError(a.t, err)

because the assertObject function is not called in the nstemplatetiers_test.go but listObjects is, so it would be verified automatically.

dperaza4dustbit

Some comments added, working on improving test to check all timestamps

dperaza4dustbit · 2024-08-09T16:36:43Z

pkg/cmd/generate/util.go

@@ -158,8 +159,11 @@ const header = `# --------------------------------------------------------------

 func writeFile(filePath string, content []byte) error {
 	// https://github.com/kubernetes/kubernetes/issues/67610
-	contentString := strings.ReplaceAll(string(content), "\n  creationTimestamp: null", "")
+	emptyCreationTimestamp := regexp.MustCompile(`\n *creationTimestamp: null`)


This works well as suggest by @MatousJobanek

What about declaring this as a top level var so that the regex doesn't have to be compiled with each invocation of this function?

I can do that yeah

dperaza4dustbit · 2024-08-09T16:39:38Z

pkg/cmd/generate/util.go

 	contentString = strings.ReplaceAll(contentString, "\nuser: {}", "")
+	// This will only apply to konflux tier template that start with metadata = {}
+	contentString = strings.ReplaceAll(contentString, "metadata:\n    objects:", "metadata: {}\n    objects:")


This fixes the metadata problem with Konflux as tiers in src already start with spec.template.metadata as {}. See here: https://github.com/redhat-appstudio/infra-deployments/blob/main/components/sandbox/tiers/src/appstudio/spacerole_admin.yaml#L3
But this won't affect our e2e nor unitest as those start with spec.template.metadata.name having a string value. Would this work?

looks good, this should work fine 👍 🚀

another option would be removing the metadata field completely. But I'll leave it up to you ;-)

Suggested change

contentString = strings.ReplaceAll(contentString, "metadata:\n objects:", "metadata: {}\n objects:")

contentString = strings.ReplaceAll(contentString, "metadata:\n objects:", "objects:")

Well tried that but ArgoCD expects metadata: {}. Will prefer to match exactly what ArgoCD expects there to avoid re-synching situation

dperaza4dustbit · 2024-08-09T16:40:52Z

pkg/cmd/generate/util.go

@@ -159,6 +159,7 @@ const header = `# --------------------------------------------------------------
 func writeFile(filePath string, content []byte) error {
 	// https://github.com/kubernetes/kubernetes/issues/67610
 	contentString := strings.ReplaceAll(string(content), "\n  creationTimestamp: null", "")
+	contentString = strings.ReplaceAll(contentString, "\n      creationTimestamp: null", " {}")


see my new approach in utils.go, seems simpler to me

dperaza4dustbit · 2024-08-09T16:41:34Z

pkg/cmd/generate/util.go

 	contentString := strings.ReplaceAll(string(content), "\n  creationTimestamp: null", "")
+	contentString = strings.ReplaceAll(contentString, "\n      creationTimestamp: null", " {}")


Took your suggestion here

MatousJobanek

it looks good overall, just waiting for the updates of the unit tests #50 (comment)

Signed-off-by: David Peraza <dperaza@redhat.com>

@MatousJobanek

Also changed the replacement of creationTimestamp at any level with regex as suggested in reviews by @MatousJobanek Signed-off-by: David Peraza <dperaza@redhat.com>

Signed-off-by: David Peraza <dperaza@redhat.com>

dperaza4dustbit · 2024-10-09T16:33:10Z

it looks good overall, just waiting for the updates of the unit tests #50 (comment)

Got it

Signed-off-by: David Peraza <dperaza@redhat.com>

dperaza4dustbit · 2024-10-10T16:27:20Z

as for unit-testing - let's either improve the util_test.go so it verifies objects at any level, or add this check:

ksctl/pkg/cmd/generate/assertion_test.go

Lines 169 to 171 in d758210

assert.True(t, strings.HasPrefix(string(file), header), "every file generated by cli should container a header")

assert.NotContains(t, string(file), "creationTimestamp")

assert.NotContains(t, string(file), "user: {}")

into

ksctl/pkg/cmd/generate/assertion_test.go

Lines 142 to 143 in d758210

objFile, err := os.ReadFile(path)

require.NoError(a.t, err)

because the assertObject function is not called in the nstemplatetiers_test.go but listObjects is, so it would be verified automatically.

added assertions to listObject method, thanks for suggestion

MatousJobanek

Looks good, thanks for addressing our comments

codecov · 2024-10-11T08:35:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.59%. Comparing base (ac3aaca) to head (fd122c3).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   69.57%   69.59%   +0.01%     
==========================================
  Files          43       43              
  Lines        2564     2565       +1     
==========================================
+ Hits         1784     1785       +1     
  Misses        589      589              
  Partials      191      191

Files with missing lines	Coverage Δ
pkg/cmd/generate/util.go	`78.16% <100.00%> (+0.15%)`	⬆️

dperaza4dustbit requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan, ranakan19, sbryzak, mfrancisc, drpaneas, metlos and fbm3307 as code owners July 15, 2024 22:59

alexeykazakov reviewed Jul 15, 2024

View reviewed changes

fbm3307 reviewed Jul 16, 2024

View reviewed changes

pkg/cmd/generate/util.go Outdated Show resolved Hide resolved

dperaza4dustbit force-pushed the fix-create-timestamp-null branch 2 times, most recently from 8636ee0 to 013d2c4 Compare July 18, 2024 17:36

dperaza4dustbit commented Jul 18, 2024

View reviewed changes

fbm3307 reviewed Jul 19, 2024

View reviewed changes

pkg/cmd/generate/util.go Outdated Show resolved Hide resolved

pkg/cmd/generate/util.go Outdated Show resolved Hide resolved

MatousJobanek reviewed Jul 22, 2024

View reviewed changes

dperaza4dustbit mentioned this pull request Jul 25, 2024

Add konflux-eaas kubesaw tier redhat-appstudio/infra-deployments#4189

Merged

dperaza4dustbit force-pushed the fix-create-timestamp-null branch from d0e0da2 to b59512e Compare August 9, 2024 16:35

dperaza4dustbit commented Aug 9, 2024

View reviewed changes

MatousJobanek reviewed Aug 21, 2024

View reviewed changes

This was referenced Aug 27, 2024

chore(RHTAPWATCH-1169): Reduce permissions in the appstudio-env tier redhat-appstudio/infra-deployments#4405

Merged

More SPI leftovers cleanup redhat-appstudio/infra-deployments#4417

Merged

hugares mentioned this pull request Oct 7, 2024

chore(KFLUXVNGD-38): remove maintainers delete permissions redhat-appstudio/infra-deployments#4660

Merged

dperaza4dustbit added 3 commits October 9, 2024 08:34

Removing creationTimestamp under Template object

062b496

Signed-off-by: David Peraza <dperaza@redhat.com>

When template.metadata is empty string switch to empty object

a495af3

Also changed the replacement of creationTimestamp at any level with regex as suggested in reviews by @MatousJobanek Signed-off-by: David Peraza <dperaza@redhat.com>

Moving creationTimestamp regex as a top level var

156189f

Signed-off-by: David Peraza <dperaza@redhat.com>

dperaza4dustbit force-pushed the fix-create-timestamp-null branch from f28d487 to 156189f Compare October 9, 2024 12:57

Added asserts to listObjects for more test coverage on creationTimestamp

fd122c3

Signed-off-by: David Peraza <dperaza@redhat.com>

MatousJobanek approved these changes Oct 11, 2024

View reviewed changes

MatousJobanek merged commit 9e9b2f4 into kubesaw:master Oct 11, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing creationTimestamp under Template object #50

Removing creationTimestamp under Template object #50

dperaza4dustbit commented Jul 15, 2024

alexeykazakov left a comment

dperaza4dustbit commented Jul 15, 2024

fbm3307 left a comment

dperaza4dustbit Jul 18, 2024

MatousJobanek Jul 22, 2024

MatousJobanek Jul 22, 2024

dperaza4dustbit Aug 9, 2024

dperaza4dustbit commented Jul 18, 2024

fbm3307 left a comment •

edited

Loading

MatousJobanek Jul 22, 2024

dperaza4dustbit Aug 9, 2024

MatousJobanek commented Jul 22, 2024

dperaza4dustbit left a comment

dperaza4dustbit Aug 9, 2024

metlos Aug 14, 2024

dperaza4dustbit Sep 10, 2024

dperaza4dustbit Oct 9, 2024

dperaza4dustbit Aug 9, 2024

MatousJobanek Aug 13, 2024

MatousJobanek Aug 13, 2024

dperaza4dustbit Sep 10, 2024

dperaza4dustbit Aug 9, 2024

dperaza4dustbit Aug 9, 2024

MatousJobanek left a comment

dperaza4dustbit commented Oct 9, 2024

dperaza4dustbit commented Oct 10, 2024

MatousJobanek left a comment

codecov bot commented Oct 11, 2024

		contentString := strings.ReplaceAll(string(content), "\n creationTimestamp: null", "")
		contentString = strings.ReplaceAll(contentString, "\n creationTimestamp: null", " {}")

Removing creationTimestamp under Template object #50

Removing creationTimestamp under Template object #50

Conversation

dperaza4dustbit commented Jul 15, 2024

alexeykazakov left a comment

Choose a reason for hiding this comment

dperaza4dustbit commented Jul 15, 2024

fbm3307 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperaza4dustbit commented Jul 18, 2024

fbm3307 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek commented Jul 22, 2024

dperaza4dustbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek left a comment

Choose a reason for hiding this comment

dperaza4dustbit commented Oct 9, 2024

dperaza4dustbit commented Oct 10, 2024

MatousJobanek left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024

Codecov Report

fbm3307 left a comment •

edited

Loading