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

docs: update getting-started and httproute guides for v1alpha2 #786

Merged
merged 1 commit into from
Aug 19, 2021
Merged

docs: update getting-started and httproute guides for v1alpha2 #786

merged 1 commit into from
Aug 19, 2021

Conversation

shaneutt
Copy link
Member

@shaneutt shaneutt commented Aug 17, 2021 •
edited
Loading

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This updates the examples in the HTTPRoute guide for v1alpha2, and does some spacing cleanup for the getting started guide (but that guide did not appear to need any special updating for v1alpha2).

Which issue(s) this PR fixes:

Resolves the first two tasks in #783

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @shaneutt. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 17, 2021
@shaneutt shaneutt mentioned this pull request Aug 17, 2021
Closed
7 tasks
@shaneutt shaneutt force-pushed the v1alpha2-httproute-docs branch 2 times, most recently from ef39da7 to 5cafba8 Compare August 17, 2021 20:39
@shaneutt shaneutt marked this pull request as ready for review August 17, 2021 20:40
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 17, 2021
@robscott
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2021
@shaneutt shaneutt force-pushed the v1alpha2-httproute-docs branch from 5cafba8 to b23c84d Compare August 17, 2021 20:46
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2021
robscott
robscott reviewed Aug 17, 2021
View reviewed changes
Copy link
Member

@robscott robscott 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 on this @shaneutt!

site-src/v1alpha2/guides/http-routing.md Outdated
@@ -33,7 +29,7 @@ Route labels and Gateway label selectors allow Routes and Gateways to be
bound to each other by their respective owners.
Copy link
Member

Choose a reason for hiding this comment

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

The paragraph above this needs to be updated to match v1alpha2 route attachment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up tearing it out and replacing this and the example yaml, the results are in c03ae32 lmkwyt 👍

examples/v1alpha2/http-routing/gateway.yaml Outdated Show resolved Hide resolved
examples/v1alpha2/http-routing/bar-httproute.yaml Show resolved Hide resolved
examples/v1alpha2/http-routing/foo-httproute.yaml Show resolved Hide resolved
@shaneutt shaneutt force-pushed the v1alpha2-httproute-docs branch from b23c84d to d96f473 Compare August 18, 2021 13:37
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 18, 2021
@shaneutt shaneutt requested a review from robscott August 18, 2021 13:56
@shaneutt
Copy link
Member Author

/assign @shaneutt

@shaneutt shaneutt mentioned this pull request Aug 18, 2021
Closed
7 tasks
@shaneutt shaneutt changed the title docs: update httproute guide for v1alpha2 docs: update getting-started and httproute guides for v1alpha2 Aug 18, 2021
robscott
robscott reviewed Aug 18, 2021
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Just a few more nits, but otherwise LGTM.

site-src/v1alpha2/guides/http-routing.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/getting-started.md Outdated
Comment on lines 3 to 5
!!! danger
This page has not been updated for v1alpha2 yet.

Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to stay in place until the directions actually allow for installing v1alpha2 CRDs

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, because this is the landing page so this should be the last one to remove its banner, makes sense to me in retrospect.

Given that part of the context here seems to be that it's the pages this page links to that are not updated yet, I added this back in b6a39a6 but I changed the wording a bit, lmkwyt 👍

examples/v1alpha2/http-routing/gateway.yaml Outdated Show resolved Hide resolved
examples/v1alpha2/http-routing/foo-httproute.yaml Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/http-routing.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/http-routing.md Outdated Show resolved Hide resolved
@shaneutt shaneutt requested a review from robscott August 18, 2021 19:23
@robscott robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 18, 2021
@youngnick
Copy link
Contributor

LGTM pending CI, nice work @shaneutt!

@shaneutt shaneutt removed their assignment Aug 19, 2021
@shaneutt shaneutt force-pushed the v1alpha2-httproute-docs branch from 4775c71 to 33d492b Compare August 19, 2021 18:34
@shaneutt
Copy link
Member Author

@robscott I think you had asked earlier for the spec to be removed from an example GatewayClass however this ended up failing CI verification for the manifest, so I put that back in. Verification is successfull locally now, and I've rebased against recent master changes and squashed. Ready for re-review.

@robscott
Copy link
Member

robscott commented Aug 19, 2021 •
edited
Loading

I think you had asked earlier for the spec to be removed from an example GatewayClass

@shaneutt sorry for the confusion there! I was actually just suggesting that the GatewayClass resource was unnecessary for that example. Most users won't be creating a new GatewayClass for each Gateway, Route, etc. In the text before and after the example, there's no mention of the GatewayClass, so I think it could safely be removed from that example.

@shaneutt shaneutt force-pushed the v1alpha2-httproute-docs branch from 33d492b to 85f8400 Compare August 19, 2021 18:49
@shaneutt
Copy link
Member Author

I think you had asked earlier for the spec to be removed from an example GatewayClass

@shaneutt sorry for the confusion there! I was actually just suggesting that the GatewayClass resource was unnecessary for that example. Most users won't be creating a new GatewayClass for each Gateway, Route, etc. In the text before and after the example, there's no mention of the GatewayClass, so I think it could safely be removed from that example.

Sounds good, that's now removed.

@robscott
Copy link
Member

Thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2021
@robscott
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, shaneutt

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 Aug 19, 2021
@k8s-ci-robot k8s-ci-robot merged commit 01666be into kubernetes-sigs:master Aug 19, 2021
@shaneutt shaneutt deleted the v1alpha2-httproute-docs branch August 19, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpeach jpeach Awaiting requested review from jpeach

@robscott robscott Awaiting requested review from robscott

Assignees

@robscott robscott

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. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shaneutt @k8s-ci-robot @robscott @youngnick