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 links to files on installation files page (#4998) #5751

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

prushh
Copy link
Contributor

@prushh prushh commented Nov 8, 2023

Use artifact macro to generate the link to the appropriate yaml files for the serving and eventing installation pages.

Fixes #4998

Use artifact macro to generate the link to the appropriate files.
Copy link

netlify bot commented Nov 8, 2023

Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 16cc872
🔍 Latest deploy log https://app.netlify.com/sites/knative/deploys/654b8d0d0b0c310008775740
😎 Deploy Preview https://deploy-preview-5751--knative.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

knative-prow bot commented Nov 8, 2023

Welcome @prushh! It looks like this is your first PR to knative/docs 🎉

@knative-prow knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 8, 2023
Copy link
Member

@ReToCode ReToCode left a comment

Choose a reason for hiding this comment

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

Thanks @prushh looks good. Let's see what the eventing folks have to say about the eventing-sugar-controller.

/hold

| [eventing-core.yaml]({{ artifact(repo="eventing",file="eventing-core.yaml")}}) | Required: Knative Eventing core components. | [eventing-crds.yaml]({{ artifact(repo="eventing",file="eventing-crds.yaml")}}) |
| [eventing-crds.yaml]({{ artifact(repo="eventing",file="eventing-crds.yaml")}}) | Required: Knative Eventing core CRDs. | none |
| [eventing-post-install.yaml]({{ artifact(repo="eventing",file="eventing-post-install.yaml")}}) | Jobs required for upgrading to a new minor version. | [eventing-core.yaml]({{ artifact(repo="eventing",file="eventing-core.yaml")}}), [eventing-crds.yaml]({{ artifact(repo="eventing",file="eventing-crds.yaml")}}) |
| [eventing-sugar-controller.yaml]({{ artifact(repo="eventing",file="eventing-sugar-controller.yaml")}}) | Reconciler that watches for labels and annotations on certain resources to inject eventing components. | [eventing-core.yaml]({{ artifact(repo="eventing",file="eventing-core.yaml")}}) |
Copy link
Member

Choose a reason for hiding this comment

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

@pierDipi @creydr is this still used? The link seems to end up in 404.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC the sugar controller can be enabled now via a config map and no additional deployment is required. Anyhow I leave it to @pierDipi for the final confirmation, that this line can be removed

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If so, we should update at least two files in addition to the one above:

  • docs/docs/install/uninstall.md -> L148-L154
  • docs/docs/install/yaml-install/eventing/install-eventing-with-yaml.md -> L231-L254

Do you think there are others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pierDipi Should I create a new PR with the changes mentioned above?

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2023
@psschwei
Copy link
Contributor

psschwei commented Nov 8, 2023

Thanks for the PR!

It might not be a bad idea to run the full build (with versions) locally just to double check the links are correct in the release branches, since the preview only builds from main and thus gets the yamls from googlestorage rather than the release branch(es)...

@prushh
Copy link
Contributor Author

prushh commented Nov 8, 2023

It might not be a bad idea to run the full build (with versions) locally just to double check the links are correct in the release branches, since the preview only builds from main and thus gets the yamls from googlestorage rather than the release branch(es)...

Sure!
I will run the full build tomorrow and check that the links on the different release branches are correct.

@psschwei
Copy link
Contributor

psschwei commented Nov 9, 2023

Actually, turns out you can't just build the release branches to test (since the release branches are pulled from github which don't have this change). But, running something like this:

KNATIVE_VERSION="v1.12.0" BUILD_VERSIONS=no ./hack/build.sh serve

will do the build in a way that you can check

http://127.0.0.1:8080/docs/install/yaml-install/serving/serving-installation-files/
http://127.0.0.1:8080/docs/install/yaml-install/eventing/eventing-installation-files/

and see that the changes should be picked up in the release branches, though you'll need to cherry pick this change for it to show up on the site (at least into the the branch of the most current release)

@prushh
Copy link
Contributor Author

prushh commented Nov 9, 2023

Looks good for both pages, the links point to the release specified by the KNATIVE_VERSION env:
knative-v1 12 0-file-links

If I understand well, should we also specify (at least) the first command from the following list?

  • /cherrypick release-1.12
  • /cherrypick release-1.11
  • /cherrypick release-1.10

@psschwei
Copy link
Contributor

psschwei commented Nov 9, 2023

should we also specify (at least) the first command from the following list?

Yes, that's right

@prushh
Copy link
Contributor Author

prushh commented Nov 9, 2023

/cherrypick release-1.12

I leave you the other two commands if you think to update old releases!

@knative-prow-robot
Copy link
Contributor

@prushh: only knative org members may request cherry-picks. You can still do the cherry-pick manually.

In response to this:

/cherrypick release-1.12

I leave you the other two commands if you think to update old releases!

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.

@psschwei
Copy link
Contributor

psschwei commented Nov 9, 2023

/cherrypick release-1.12

@knative-prow-robot
Copy link
Contributor

@psschwei: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.12

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.

@prushh
Copy link
Contributor Author

prushh commented Nov 9, 2023

Sorry, I didn't know it was a limited-use command.

Thanks @psschwei!

Copy link
Member

@pierDipi pierDipi 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

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2023
Copy link

knative-prow bot commented Dec 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, prushh

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2023
@pierDipi
Copy link
Member

pierDipi commented Dec 5, 2023

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2023
@knative-prow knative-prow bot merged commit df08d10 into knative:main Dec 5, 2023
19 checks passed
@knative-prow-robot
Copy link
Contributor

@psschwei: new pull request created: #5786

In response to this:

/cherrypick release-1.12

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.

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. lgtm 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.

Adds links to files on the installation files pages
6 participants