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

Follow-on to initial feedback, address some unresolved blocks #1

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

justinsb
Copy link
Collaborator

No description provided.

@justinsb justinsb force-pushed the kubectl_apply_prune branch from 4a679f9 to d1a6bae Compare January 18, 2023 15:50
* `kubectl.` for kubectl
* `helm.` for helm
* `kpt.` for kpt
* `uid.` for where the suffix is the UID of the applyset object (e.g. `uid.e049464e-4583-4642-9649-93dcb0e96bd4`)
Copy link
Owner

Choose a reason for hiding this comment

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

Did you see this comment? kubernetes#3661 (comment). A long time ago, I saw a case where encoding a UID as an object reference like this backfired majorly, tldr because it did not line up with user-facing concept of object identity, so I'm really hesitant to recommend this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I had seen that a few days ago but forgot. My chain of thought here was to provide some recommended structure for the applyset id, and to reserve a few values rather than setting up a format registry. I think we can recommend it separately from reserving it, but I realize this might be more than we want to do. I don't think we'd fall into the traditional drawback of UID, which is that it changes over a cluster backup and restore. If the applyset label didn't match the object ID, I think we might log a warning, but I don't think we would try to change the applyset label. We do have to do a two-step apply to use the UID though, because we can't know the UID until we apply the object, so that's not ideal.

So all that said, I'm happy to just drop uid here, we can always add it later if we need it (and I don't think we necessarily do)

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, in the spirit of you being an equal co-author who happens to not have push permissions, I'm going to go ahead and merge this, and we can make the decision on the "real" PR. I agree with everything else, and I'm going to think more about this one. One scenario I'm wondering about is when folks intentionally migrate from one tool to another, which we are claiming should be easier with this common foundation. Then the tool-specific naming would be undesirable right?

(as a sidenote for the id option, it would be worth mentioning any precedents for that format, e.g. kubectl get's "deployments.v1.apps", and Kustomize has something similar for GVKNN references albeit just in error messages)

@KnVerey KnVerey merged commit 5371185 into KnVerey:kubectl_apply_prune Jan 18, 2023
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.

2 participants