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

Update the owning-inventory for existing objects during migration #1318

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

Liujingfang1
Copy link
Contributor

After we add the InventoryPolicy in the cli-utils library, we need to update the migrate command so that the owning-inventory annotations are pointed to the ResourceGroup object. The change is to loop over the resources that are included in the ConfigMap inventory object and change their owning-inventory annotation to the new ResourceGroup CR.

When updating the owning-inventory annotation, we also update the last-applied-configuration annotation.

Fix #1306 This is a different approach compared with #1307.

@Liujingfang1
Copy link
Contributor Author

/assign @mortent @seans3

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.

Some small questions, but I think this looks good. I think this approach is simpler and safer than in #1307
I'll let @seans3 take a look too

commands/migratecmd.go Outdated Show resolved Hide resolved
if options == nil {
options = &metav1.UpdateOptions{}
}
_, err = r.Update(ctx, obj, *options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this result in an UPDATE or a PATCH call to the apiserver? Just want to make sure we do this the same way as a regular apply operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an UPDATE call. I didn't use the PATCH call since we should only change the metadata.annotation. Only two annotations are updated: the owning-inventory and the last-applied-config annotation. If we compute them correctly, we don't need to use the PATCH call.

Copy link
Contributor

Choose a reason for hiding this comment

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

For more context: the Update call translates to a REST Put with the entire object in the body.

Copy link
Contributor

@seans3 seans3 left a comment

Choose a reason for hiding this comment

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

/lgtm

if options == nil {
options = &metav1.UpdateOptions{}
}
_, err = r.Update(ctx, obj, *options)
Copy link
Contributor

Choose a reason for hiding this comment

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

For more context: the Update call translates to a REST Put with the entire object in the body.

@seans3
Copy link
Contributor

seans3 commented Jan 6, 2021

Should we squash the commits before merging?

commit 3422bf620ace313c5b806986579d498d48404f04
Author: Jingfang Liu <jingfangliu@google.com>
Date:   Mon Dec 28 13:55:32 2020 -0800

    change the order of updating owning-inventory annotation and creating ResourceGorup CR

commit be90e247a8729aa7321bd3b23541894805f26cc4
Author: Jingfang Liu <jingfangliu@google.com>
Date:   Tue Dec 22 16:16:46 2020 -0800

    fix lint error

commit e9c9c3869268424184eb07647c1f47790c468e28
Author: Jingfang Liu <jingfangliu@google.com>
Date:   Tue Dec 22 14:44:52 2020 -0800

    update the last-applied-configuration properly

commit 901528a3ff3b89f4fc163d09d75dc02ac04cf4e0
Author: Jingfang Liu <jingfangliu@google.com>
Date:   Tue Dec 22 14:16:41 2020 -0800

    update owning-inventory annotation during migration
@Liujingfang1
Copy link
Contributor Author

@seans3 I squashed the commits. Let's wait for the tests to pass.

@seans3 seans3 merged commit 8201c64 into kptdev:master Jan 6, 2021
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.

Regression: pruning fails for ResourceGroup inventory object
3 participants