-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 consistent naming for API constants #7618
⚠️ Use consistent naming for API constants #7618
Conversation
Hi @chiukapoor. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
// external objects(bootstrap and infrastructure providers). | ||
ClusterLabelName = "cluster.x-k8s.io/cluster-name" | ||
ClusterNameLabel = "cluster.x-k8s.io/cluster-name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renames should be eventually all added to the migration doc.
The migration doc should be created by the release team in 1-2 weeks: https://github.com/kubernetes-sigs/cluster-api/blob/d146f6d9e264241c5db29e8ecf99a390ab97807d/docs/release/release-tasks.md#add-docs-to-collect-release-notes-for-users-and-migration-notes-for-provider-implementers
Theoretically we could do it here already but I would like to give the first release team a chance to try out our new release process, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also in general hold merging this PR until at least after the v1.3.0 release (1st December) just to ensure it doesn't interfere with the release / cherry-picks until then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will make sure the PR is on hold until at least after the v1.3.0 release.
In general looks fine +/- the comments above. Given the amount of files that have to be touched for those renames. Maybe you can do further renames for now in separate commits and then we can see how big the impact overall is and potentially decide to move individual commits to separate PRs? Although this label should be pretty much the one with the most usage. |
Sure @sbueringer, Will make commits to this PR for now, and later we may decide upon if it requires separate PRs 👍 |
Sounds perfect, thank you! |
5a34e09
to
815aff5
Compare
Updated the constant from |
80c5d19
to
124f2a2
Compare
Updated the constant from |
@sbueringer, While working on label |
No that's fine. The tiltfile is using the tilt-prepare binary at runtime. tilt-prepare does not generate the Tiltfile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm from my side
api/v1beta1/common_types.go
Outdated
// This label allows to easily identify all the components belonging to a provider; the clusterctl | ||
// tool uses this label for implementing provider's lifecycle operations. | ||
ProviderLabelName = "cluster.x-k8s.io/provider" | ||
ProviderLabel = "cluster.x-k8s.io/provider" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: should this be
ProviderLabel = "cluster.x-k8s.io/provider" | |
ProviderNameLabel = "cluster.x-k8s.io/provider" |
@sbueringer opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure
The following keeps the mapping from label string to constant name simple and straight-forward:
I would suggest to only include a Name in the constant name if the label actually contains name. In that case we should include it before Label and not after.
#7611
If the label string itself is wrong ideally we would fix the label string to cluster.x-k8s.io/provider-name
, but it's a non-trivial effort.
I think for better readability of consumer code I would make an exception to the naming rule and go with your suggestion (ProviderNameLabel
) for this label. As far as I can tell it's the only label where we have this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will inculcate the changes.
6009a5e
to
fade95b
Compare
fade95b
to
f9903f8
Compare
b37acb0
to
01cd408
Compare
Hello @sbueringer and @fabriziopandini, I have fixed all the constants with inconsistency mentioned in #7611 and have made the requested change of PTAL |
Please squash commits, I will take a look right after |
Signed-off-by: Chirayu Kapoor <dev.csociety@gmail.com>
a751073
to
f2ba999
Compare
@chiukapoor: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@fabriziopandini The commits have been squashed, I have also fixed the below mismatch in the comment and function name. cluster-api/internal/controllers/topology/cluster/cluster_controller_test.go Lines 1040 to 1042 in 709d848
Also, as per the update from @sbueringer in reply #7618 (comment), the PR merge needs to be on hold until 1.3.0 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit from my side
Also, @sbueringer what about marking this PR as
// This label allows to easily identify all the components belonging to a provider; the clusterctl | ||
// tool uses this label for implementing provider's lifecycle operations. | ||
ProviderLabelName = "cluster.x-k8s.io/provider" | ||
ProviderNameLabel = "cluster.x-k8s.io/provider" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per @sbueringer this should be
ProviderNameLabel = "cluster.x-k8s.io/provider" | |
ProviderLabel = "cluster.x-k8s.io/provider" |
(the label is just provider, not provider-name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the @sbueringer suggestion in reply #7618 (comment) I made this exception. Please do let me know if I should revert it?
I think for better readability of consumer code I would make an exception to the naming rule and go with your suggestion (ProviderNameLabel) for this label. As far as I can tell it's the only label where we have this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with ProviderNameLabel
for the stated reason. @fabriziopandini I'm also fine with ProviderLabel
if you prefer it.
Yup let's do that. /retitle |
/lgtm I think we can merge this before the migration doc. Let's just keep the corresponding issue open and do a follow-up PR for: https://github.com/kubernetes-sigs/cluster-api/pull/7618?notification_referrer_id=NT_kwDOAEckWLI0OTIxNDA3OTc1OjQ2NjIzNjA¬ifications_query=repo%3Akubernetes-sigs%2Fcluster-api#discussion_r1031997700 |
@sbueringer should I create another PR attached to #7611 for the migration doc? If yes, what should be added to the doc? Also, how may I resolve the |
Let's just add a comment to #7611 for now and make sure that the current PR does not close the issue (change "fixes" in the pr description to "Part of")
You don't have to resolve it. It's just an indiciation for reviewers that we change an API. It's optional for merge. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Chirayu Kapoor <dev.csociety@gmail.com>
Signed-off-by: Chirayu Kapoor dev.csociety@gmail.com
What this PR does / why we need it:
Fixes the inconsistent constant names
Which issue(s) this PR fixes
Fixes #7611