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

📖 Update docs for beta ClusterResourceSet #10289

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

mboersma
Copy link
Contributor

@mboersma mboersma commented Mar 19, 2024

What this PR does / why we need it:

Updates the documentation (and two manifests) to reflect the status of the ClusterResourceSet experimental feature as Beta and true by default.

When creating a PR to promote the MachinePools experimental feature to Beta / true, it seemed there was similar cleanup that should be done to CRS.

Which issue(s) this PR fixes:
N/A, but see also #10141

/area documentation

@k8s-ci-robot k8s-ci-robot added area/documentation Issues or PRs related to documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 19, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 19, 2024
@mboersma mboersma changed the title 🌱 Update docs for beta ClusterResourceSet 📖 Update docs for beta ClusterResourceSet Mar 19, 2024
@mboersma
Copy link
Contributor Author

/area clusterresourceset

@k8s-ci-robot k8s-ci-robot added the area/clusterresourceset Issues or PRs related to clusterresourcesets label Mar 19, 2024
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 19, 2024
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Was there a general agreement to promote CRS to beta as well? Do we have an issue covering what's needed here like we do for MachinePools - i.e. #9005?

@mboersma
Copy link
Contributor Author

AFAICT ClusterResourceSet has been beta already for quite a while:

// ClusterResourceSet is a feature gate for the ClusterResourceSet functionality.
//
// alpha: v0.3
// beta: v0.4
ClusterResourceSet featuregate.Feature = "ClusterResourceSet"

var defaultClusterAPIFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
// Every feature should be initiated here:
MachinePool: {Default: false, PreRelease: featuregate.Alpha},
ClusterResourceSet: {Default: true, PreRelease: featuregate.Beta},
ClusterTopology: {Default: false, PreRelease: featuregate.Alpha},

So I thought this PR was just catching up with reality by marking it beta in the "experimental features" doc where it was still labeled alpha, and removing all the places where we say "don't forget to turn on this experimental feature." But I may be mistaken.

@killianmuldoon
Copy link
Contributor

I'm really not sure - but this is something that should probably have a tracking issue and be brought up at the community meeting IMO - I'm not sure if a decision was made to turn this on by default in this way but it would be good to make sure.

@mboersma
Copy link
Contributor Author

mboersma commented Mar 20, 2024

/hold

this is something that should probably have a tracking issue and be brought up at the community meeting

I added this as a discussion topic for the March 27th CAPI community meeting.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 20, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024
@mboersma
Copy link
Contributor Author

/hold cancel

At today's office hours meeting, consensus was that CRS has been beta and that it was an oversight that experimental-features.md didn't reflect this.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024
@mboersma
Copy link
Contributor Author

Updated to include great review comments from @neolit123.

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

pls unhold if no additional review is pending.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 03f338a8c18b267488151656ab6311ca2cf28164

@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 27, 2024
@sbueringer
Copy link
Member

sbueringer commented Mar 28, 2024

Thank you very much for taking care of this!

/hold cancel
/approve

I would not backport this as I think we shouldn't go from disabled per default to enabled per default in a patch release (for folks using our manager.yaml / components.yaml / clusterctl)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2024
@fabriziopandini
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, neolit123, sbueringer

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:
  • OWNERS [fabriziopandini,neolit123,sbueringer]

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 merged commit 1e8302b into kubernetes-sigs:main Mar 28, 2024
22 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.7 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterresourceset Issues or PRs related to clusterresourcesets area/documentation Issues or PRs related to documentation 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
Development

Successfully merging this pull request may close these issues.

6 participants