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

kpt live apply should install resource group #2092

Closed
phanimarupaka opened this issue May 24, 2021 · 9 comments
Closed

kpt live apply should install resource group #2092

phanimarupaka opened this issue May 24, 2021 · 9 comments
Assignees
Labels
area/live customer deep engagement enhancement New feature or request p1 size/S 1 day triaged Issue has been triaged by adding an `area/` label

Comments

@phanimarupaka
Copy link
Contributor

In kpt 1.0, installing resource group can be a pre step for live commands. Users should not be prompted to install resource group explicitly.

@phanimarupaka phanimarupaka added enhancement New feature or request area/live triaged Issue has been triaged by adding an `area/` label labels May 24, 2021
@phanimarupaka phanimarupaka added this to To do in kpt kanban board via automation May 24, 2021
@phanimarupaka phanimarupaka added this to the v1.0 m3 milestone May 24, 2021
@frankfarzan
Copy link
Contributor

frankfarzan commented May 24, 2021

There is a flag (--install-resource-group) that specifies this behavior (whether to automatically install or not), so the question is what is the default value of this flag. We have gone back and forth on this. There was a concern that a "preview" command was installing the CRD. I think with the recent change to using --dry-run instead of preview, having the default be automatically install is the more reasonable behavior. The additional friction caused by not automatically installing the CRD is the bigger of the two evils.

@frankfarzan
Copy link
Contributor

cc @mortent @seans3

@mikebz mikebz added the size/S 1 day label May 24, 2021
@mortent
Copy link
Contributor

mortent commented May 24, 2021

I still don't think we should install anything in the cluster when users provide the --dry-run flag. I don't see how changing from a separate preview command to a --dry-run flag changes this.

@mikebz mikebz added customer deep engagement p1 labels May 24, 2021
@frankfarzan
Copy link
Contributor

frankfarzan commented May 25, 2021

Previously, preview was a prominent part of the user flow. Because of the limitation in how dry-run works, we made it a flag. This is a trade-off, and we should favor streamlining the UX for apply.

Related: The Quickstart guide actually glosses over this implication: https://kpt.dev/book/01-getting-started/02-quickstart?id=apply-the-package. Having to run init and install-crd before apply is too cumbersome if you're coming from a tool like kubectl.

One possible way to ask permission for installing the CRD for dry-run:

  • If --dry-run is specified, then require --install-resource-group flag if the resource is missing from the cluster. Otherwise, error out.
  • If --dry-run is not specified, then install automatically.

However, having default values of flags change is somewhat convoluted.

@seans3
Copy link
Contributor

seans3 commented May 25, 2021

I agree with @mortent: we should not be mutating during a dry-run

@mikebz mikebz moved this from To do to In progress in kpt kanban board May 25, 2021
@frankfarzan
Copy link
Contributor

What's the interaction of --dry-run and --install-resource-group: https://kpt.dev/reference/live/apply/
I think at least for sake of scriptability, you want to allow the user to explicitly allow installing the CRD if they want.

@frankfarzan frankfarzan changed the title kpt live init, preview, apply should install resource group kpt live apply should install resource group May 26, 2021
@seans3
Copy link
Contributor

seans3 commented Jun 1, 2021

For scriptability, we have the command kpt live install-resource-group if users want to install the CRD, then do an apply --dry-run. So I don't think we should allow the flags --dry-run with install-resource-group; this should be an error.

@mortent
Copy link
Contributor

mortent commented Jun 1, 2021

So we have had discussions around this. I think we all agree that mutating the cluster by default during dry-run would be surprising behavior, but we also want to reduce the friction for people adopting kpt. I think the compromise here is that we let the --install-resource-group flag default to false when the --dry-run flag is provided, but default to true otherwise. Having different default values isn't ideal, but I think it is the best tradeoff here.

The result will be:

  • kpt does not install the resource-group CRD by default during dry-run.
  • Users can override this by adding the install-resource-group flag. So kpt live apply --dry-run --install-resource-group will install the resource-group CRD.
  • kpt live apply (without dry-run) will install the resource-group CRD by default.

The only change required here is to have the --install-resource-group flag default to true if it is not a dry-run. The behavior for the --install-resource-group flag currently works the correct way.

@seans3
Copy link
Contributor

seans3 commented Jun 3, 2021

/assign @mortent

@seans3 seans3 assigned mortent and unassigned seans3 Jun 3, 2021
@mortent mortent moved this from In progress to In Review in kpt kanban board Jun 3, 2021
@mortent mortent closed this as completed Jun 7, 2021
kpt kanban board automation moved this from In Review to Done Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/live customer deep engagement enhancement New feature or request p1 size/S 1 day triaged Issue has been triaged by adding an `area/` label
Projects
Development

No branches or pull requests

5 participants