Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move resource version annotation to a different file #4018

Merged
merged 1 commit into from
Aug 12, 2023

Conversation

bharat-m1
Copy link
Contributor

Moving resource version annotation to a new file. The newly added file can be used for storing all the metadata going forward. It is added and removed from the client side and will not be present in porch.

Please refer this for more info: #4017

@bharat-m1 bharat-m1 requested a review from a team as a code owner August 8, 2023 09:17
@bharat-m1 bharat-m1 requested a review from mortent August 8, 2023 09:17
commands/alpha/rpkg/pull/command_test.go Outdated Show resolved Hide resolved
commands/alpha/rpkg/pull/command_test.go Outdated Show resolved Hide resolved
pkg/api/kptfile/v1/types.go Outdated Show resolved Hide resolved
@bharat-m1 bharat-m1 force-pushed the kpt-meta-data branch 2 times, most recently from 3295ee5 to 609bfb3 Compare August 9, 2023 09:32
Copy link
Contributor

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change but otherwise LGTM.

commands/alpha/rpkg/util/common.go Outdated Show resolved Hide resolved
@johnbelamaric
Copy link
Contributor

The commit hash will change every run, so you probably need to tweak the test code to strip those out before doing deltas. Timestamp too. I had to do that with the prior version.

You may also want to not store all the annotations (in the actual code, not the test code), but I didn't mention it before because it's harmless that they are there; they are ignored by your push code, and the file is hidden.

@bharat-m1
Copy link
Contributor Author

bharat-m1 commented Aug 10, 2023

The commit hash will change every run, so you probably need to tweak the test code to strip those out before doing deltas. Timestamp too. I had to do that with the prior version.

You may also want to not store all the annotations (in the actual code, not the test code), but I didn't mention it before because it's harmless that they are there; they are ignored by your push code, and the file is hidden.

Additional things I can find are creationTimestamp & uid. Instead of removing specific annotations, I hope we can dump them only. As we have made it a hidden file it should not affect the user too.
Let me know if that is fine or we want to remove some of them.

Copy link
Contributor

@mortent mortent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small comment, but otherwise looks good.

@@ -113,7 +113,7 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
return errors.E(op, err)
}

if err := util.AddResourceVersionAnnotation(&resources); err != nil {
if err := util.AddRevisionMetaData(&resources); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We should use consistent naming, so this should probably be AddRevisionMetadata.

The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
@johnbelamaric johnbelamaric merged commit 571149d into kptdev:main Aug 12, 2023
16 checks passed
johnbelamaric pushed a commit to mortent/kpt that referenced this pull request Sep 18, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants