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

use preprocessing to handle the first time apply with inventorypolicy #1382

Merged
merged 3 commits into from
Feb 1, 2021

Conversation

Liujingfang1
Copy link
Contributor

With inventory-policy flags added, users have two options: strict and adopt. strict means an object can be updated/pruned only when the owning-inventory annotation matches with the current inventory object. adopt means that an object can be updated/pruned when the owning-inventory annotation matches or this annotation is not present. The default option is strict.

When package was applied by previous version of kpt, which doesn't have the inventory-policy flag. We need to handle the first time apply when user switches to the new version of kpt. In the first time apply, the inventory-policy should be adopt so that the resources will be added the owning-inventory annotation.

In this change, the first time apply is detected by checking the label "apps.kubernetes.io./managed-by". If this label is missing, we treat it as the first time apply and add this label to the inventory object.

This PR depends on kubernetes-sigs/cli-utils#316

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.

Small question about consts

commands/applycmd.go Outdated Show resolved Hide resolved
@Liujingfang1
Copy link
Contributor Author

@seans3 Addressed the comments. It's ready for another review.

@mortent
Copy link
Contributor

mortent commented Jan 28, 2021

I think this looks good. Would the use of cobra command wrappers be simplified if we just implement the commands in kpt instead of cli-utils?
And is there a way we can add tests for some of this?

@Liujingfang1
Copy link
Contributor Author

Liujingfang1 commented Jan 28, 2021

I think this looks good. Would the use of cobra command wrappers be simplified if we just implement the commands in kpt instead of cli-utils?

Yes, it would simplify the implementation a lot. We don't need to inject the preprocessing from the wrapper to the actual cli.

And is there a way we can add tests for some of this?

I'm also thinking about this. The existing e2e test, specifically the one for testing kpt live of different kpt versions, covers the first-time apply case. Besides that, we can add a unit test for the PreProcess function.

@Liujingfang1
Copy link
Contributor Author

@mortent I added unit tests for the pre processing function. PTAL

@seans3
Copy link
Contributor

seans3 commented Jan 29, 2021

looks good to me. I'll let Morten also sign-off.

@seans3
Copy link
Contributor

seans3 commented Jan 29, 2021

/lgtm

@seans3 seans3 requested a review from mortent January 29, 2021 18:28
@mortent mortent merged commit 8695bbb into kptdev:master Feb 1, 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.

None yet

3 participants