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

📖 fix file name of clusterclass proposal #6234

Merged

Conversation

schrej
Copy link
Member

@schrej schrej commented Mar 2, 2022

The date at the beginning contains an extra digit, which breaks sorting when adding newer proposals.

Since the PR validation complains that this description is a little short, I'll add this line to hopefully satisfy it 😄

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 2, 2022
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 2, 2022
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.

/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 2, 2022
@sbueringer
Copy link
Member

sbueringer commented Mar 2, 2022

Q: Do we have any links to the proposal that we should fix? (I'm aware we can't fix them all, but maybe let's check with grep.app if there are some best-effort fixes we should make)

EDIT: looks like only in our repo, but we just fix them: https://grep.app/search?q=202105256-cluster-class-and-managed-topologies.md

(link checker can't find those broken links because they will only break after merge)

@schrej schrej force-pushed the fix/clusterclass-proposal-name branch from dc5e5ba to 78dd064 Compare March 2, 2022 12:47
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2022
@schrej
Copy link
Member Author

schrej commented Mar 2, 2022

GitHub Code Search found two more that aren't just forks.

@killianmuldoon
Copy link
Contributor

Broken links look fine to me -they're just a result of updating the links here.

/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 2, 2022
@fabriziopandini
Copy link
Member

I think we are referencing this from the Kubernetes blog...

@schrej
Copy link
Member Author

schrej commented Mar 2, 2022

The Introducing ClusterClass and Managed Topologies in Cluster API blog post doesn't mention it.

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Mar 2, 2022

But it does link https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-classes.html (which is dead). I'll add a cluster-class reference page with links to that address in main. I guess we might have to backport to 1.1 to get it in the latest version of the book though, right @sbueringer?

@schrej
Copy link
Member Author

schrej commented Mar 2, 2022

According to GitHub code search there is no reference to 202105256-cluster-class-and-managed-topologies.md in kubernetes or kubernetes-sigs apart from the ones in this repo.

@sbueringer
Copy link
Member

But it does link https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-classes.html (which is dead). I'll add a cluster-class reference page with links to that address in main. I guess we might have to backport to 1.1 to get it in the latest version of the book though, right @sbueringer?

Yes https://cluster-api.sigs.k8s.io/ is current release-1.1.

@sbueringer
Copy link
Member

sbueringer commented Mar 2, 2022

@killianmuldoon I'm not sure if we should add another page so that the old link still works. We already have that page: https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-class/index.html

I would prefer fixing the link in the Kubernetes blog post

Independent of that. I think we can merge the current PR
/lgtm
(I think we can and have to ignore the link checker by just clicking on the merge button (one of the maintainers))

@killianmuldoon
Copy link
Contributor

I was thinking of moving that index page to the location of the old cluster-classes.md which would minimally fix the issue and only slightly disturb the structure.

@sbueringer
Copy link
Member

sbueringer commented Mar 2, 2022

I was thinking of moving that index page to the location of the old cluster-classes.md which would minimally fix the issue and only slightly disturb the structure.

Sounds good!

Probably it's even more correct as the other pages on that level are also like this and not inside of sub-paths.

@sbueringer
Copy link
Member

/lgtm

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Thanks for the work for searching/fixing for broken links
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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 /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 7, 2022
@sbueringer sbueringer merged commit a367af9 into kubernetes-sigs:main Mar 7, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Mar 7, 2022
@sbueringer
Copy link
Member

I merged it manually, given that it would have been impossible because of the linkchecker otherwise.

@schrej schrej deleted the fix/clusterclass-proposal-name branch March 7, 2022 19:57
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. 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants