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.

Sign up for GitHub

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

Jump to bottom

📖 Fix v1alpha4 references in clusterctl comments #8234

Merged
merged 1 commit into from
Mar 8, 2023
Merged

📖 Fix v1alpha4 references in clusterctl comments #8234

merged 1 commit into from
Mar 8, 2023

Conversation

killianmuldoon
Copy link
Contributor

@killianmuldoon killianmuldoon commented Mar 6, 2023 •
edited
Loading

Fix up a couple of references to v1alpha4 in the clusterctl code.

Fixes #

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 6, 2023
CecileRobertMichon
CecileRobertMichon reviewed Mar 6, 2023
View reviewed changes
cmd/clusterctl/api/v1alpha3/provider_type.go Outdated
@@ -50,7 +50,7 @@ type Provider struct {
// WatchedNamespace indicates the namespace where the provider controller is watching.
// If empty the provider controller is watching for objects in all namespaces.
//
// Deprecated: in clusterctl v1alpha4 all the providers watch all the namespaces; this field will be removed in a future version of this API
// Deprecated: in Cluster API v1alpha4 and above all providers watch all the namespaces; this field will be removed in a future version of this API
Copy link
Contributor

Choose a reason for hiding this comment

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

should this really say Cluster API? AFAIK clusterctl is still on v1alpha3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is related to the CAPI contract version, not the clusterctl version which is why I updated this. AFAIU it's related to https://cluster-api.sigs.k8s.io/developer/providers/v0.3-to-v0.4.html#multi-tenancy

Copy link
Member

@sbueringer sbueringer Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like in our current code we always set this field to an empty string and never read it (except in an equals func, but I didn't find a usage for the equals func). So basically it seems to be dead code today.

I have no idea if the watch behavior was rather coupled to the clusterctl v1alpha3 API or the v1alpha3 contract.

My guess is, at this point there was no difference between:

  • clusterctl v1alpha3
  • contract v1alpha3
  • other API types v1alpha3

I think technically there is no such thing as "all providers in Cluster API v1alpha4 and above". Cluster API itself doesn't have a global version which applies to core and all providers. The only thing that comes close is the contract version.

I would probably lean towards referring to the Cluster API v1alpha4 contract.

So roughly: "all providers complying with the Cluster API v1alpha4 contract or above watch all namespaces"

@sbueringer
Copy link
Member

Overall lgtm from my side. No idea what we want to do with this: #8234 (comment)

@killianmuldoon
Copy link
Contributor Author

Update the comment on WatchNamespace to refer directly to the contract. We could, alternatively, remove the explanation if we're not confident that's the meaning of the original comment.

@sbueringer
Copy link
Member

Fine for me!
lgtm pending regen

@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6303687f0d40f105d1bcde39bcf9c48ccd0346a7

@CecileRobertMichon
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit 860536b into kubernetes-sigs:main Mar 8, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

@Jont828 Jont828 Awaiting requested review from Jont828

Assignees

@sbueringer sbueringer

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
4 participants
@killianmuldoon @sbueringer @k8s-ci-robot @CecileRobertMichon