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

Build client docs #4623

Merged
merged 6 commits into from
Jan 13, 2022
Merged

Build client docs #4623

merged 6 commits into from
Jan 13, 2022

Conversation

psschwei
Copy link
Contributor

@psschwei psschwei commented Jan 10, 2022

Proposed Changes

Add documentation for the kn client to the docs.

Fixes #4430

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2022
@netlify
Copy link

netlify bot commented Jan 10, 2022

✔️ Deploy Preview for knative ready!

🔨 Explore the source changes: 3d7cccb

🔍 Inspect the deploy log: https://app.netlify.com/sites/knative/deploys/61df33e4d4963500079f2b61

😎 Browse the preview: https://deploy-preview-4623--knative.netlify.app/development/reference/client

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 10, 2022
@psschwei
Copy link
Contributor Author

psschwei commented Jan 10, 2022

On test failures:

  • Verify mkdocs strict fails because the client docs aren't in the repo by default (they're copied over as part of the build process). Perhaps we need to update the mkdocs-strict-verify job? @csantanapr @snneji any objections?

  • The various netlify jobs are failing with a cannot find package "." error:

3:08:36 PM: + pushd /tmp/tmp.Z37VtDIguA/client-main
3:08:36 PM: /tmp/tmp.Z37VtDIguA/client-main /opt/build/repo
3:08:36 PM: + go run hack/generate-docs.go out
3:08:38 PM: vendor/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go:21:2: cannot find package "." in:
3:08:38 PM: 	/tmp/tmp.Z37VtDIguA/client-main/vendor/io/fs
3:08:38 PM: ​
3:08:38 PM: ────────────────────────────────────────────────────────────────
3:08:38 PM:   "build.command" failed                                        
3:08:38 PM: ────────────────────────────────────────────────────────────────

The build script works fine on my local system, so not sure what exactly is the problem on the remote... will keep looking into it.
Update: seems to be something on the Netlify side, but I know very little about that side of things...

@psschwei psschwei changed the title [WIP] Build client docs Build client docs Jan 10, 2022
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2022
@psschwei
Copy link
Contributor Author

Update: looks like I need to make sure I'm using go1.16 for all these builds...

@psschwei
Copy link
Contributor Author

#4624 fixes the Verify failure

Copy link
Contributor

@snneji snneji 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 adding this! Just a few items in the nav need to be tweaked.

config/nav.yml Outdated
Comment on lines 276 to 283
- kn source: reference/client/kn_source.md
- kn source create: reference/client/kn_source_apiserver_create.md
- kn source delete: reference/client/kn_source_apiserver_delete.md
- kn source describe: reference/client/kn_source_apiserver_describe.md
- kn source apiserver:
- kn source apiserver: reference/client/kn_source_apiserver.md
- kn source apiserver list: reference/client/kn_source_apiserver_list.md
- kn source apiserver update: reference/client/kn_source_apiserver_update.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- kn source: reference/client/kn_source.md
- kn source create: reference/client/kn_source_apiserver_create.md
- kn source delete: reference/client/kn_source_apiserver_delete.md
- kn source describe: reference/client/kn_source_apiserver_describe.md
- kn source apiserver:
- kn source apiserver: reference/client/kn_source_apiserver.md
- kn source apiserver list: reference/client/kn_source_apiserver_list.md
- kn source apiserver update: reference/client/kn_source_apiserver_update.md
- kn source: reference/client/kn_source.md
- kn source apiserver:
- kn source apiserver: reference/client/kn_source_apiserver.md
- kn source apiserver create: reference/client/kn_source_apiserver_create.md
- kn source apiserver delete: reference/client/kn_source_apiserver_delete.md
- kn source apiserver describe: reference/client/kn_source_apiserver_describe.md
- kn source apiserver list: reference/client/kn_source_apiserver_list.md
- kn source apiserver update: reference/client/kn_source_apiserver_update.md

Copy link
Contributor

Choose a reason for hiding this comment

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

APIServerSource create, delete, and describe links need to be moved down under the kn source apiserver section.

config/nav.yml Outdated
- kn source apiserver: reference/client/kn_source_apiserver.md
- kn source apiserver list: reference/client/kn_source_apiserver_list.md
- kn source apiserver update: reference/client/kn_source_apiserver_update.md
- kn source binding: reference/client/kn_source_binding.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- kn source binding: reference/client/kn_source_binding.md
- kn source binding:
- kn source binding: reference/client/kn_source_binding.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Create a section for kn source binding

@snneji
Copy link
Contributor

snneji commented Jan 11, 2022

I'm wondering if it is possible to have some of these on the same page? For example, grouping all kn broker commands on one page with sections for create, delete, describe, list. Maybe doing the same for all of the nested groups of commands like kn channel, kn source etc?

I don't know if this is possible since all the files in the client repo are on separate pages.

@psschwei
Copy link
Contributor Author

I'm wondering if it is possible to have some of these on the same page? For example, grouping all kn broker commands on one page with sections for create, delete, describe, list. Maybe doing the same for all of the nested groups of commands like kn channel, kn source etc?

I don't know if this is possible since all the files in the client repo are on separate pages.

@rhuss can probably answer better than I can, but my understanding is that the change would need to be made on the source files, unless we wanted to try concatenating them in the build script (though I'd really rather not)

@psschwei
Copy link
Contributor Author

Discussed in WG: using a single page that links to the client/docs/cmd/kn.md in the reference section (with a variable for the branch to be filled when building the site)

@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 11, 2022
@psschwei
Copy link
Contributor Author

Linking to Github proved to be a lot easier than I thought... @snneji let me know if the page I added looks ok or if the text should be tweaked a bit.

@rhuss
Copy link
Contributor

rhuss commented Jan 12, 2022

The preview referenced above does not reflect the recent changes. Do we need to regenerate the preview or is this done automatically ?

@psschwei
Copy link
Contributor Author

psschwei commented Jan 12, 2022

Looks like the preview didn't default to the development branch... here's a proper link: https://deploy-preview-4623--knative.netlify.app/development/reference/client/

@rhuss
Copy link
Contributor

rhuss commented Jan 12, 2022

I think this is a first step in the right direction (so happy to merge this), but eventually, I really would love a tighter and more seamless integration that is easier to consume for the reader.

Let me check if I can consolidate the markdown generation into fewer pages and generate a nice drill-down navigation.

@rhuss
Copy link
Contributor

rhuss commented Jan 12, 2022

FYI, if I select 1.1 in the rendered preview, I don't see a "Client" item in the "Reference" navigation bar (guess this will work only for future versions ?)

@psschwei
Copy link
Contributor Author

I think this is a first step in the right direction (so happy to merge this), but eventually, I really would love a tighter and more seamless integration that is easier to consume for the reader.

+1

FYI, if I select 1.1 in the rendered preview, I don't see a "Client" item in the "Reference" navigation bar (guess this will work only for future versions ?)

It needs to get cherry picked into the different release branches, which we can do once we merge.

Comment on lines 3 to 5
Please see the `kn` documentation available at:

[{{ clientdocs() }}]({{ clientdocs() }}){target=_blank}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Please see the `kn` documentation available at:
[{{ clientdocs() }}]({{ clientdocs() }}){target=_blank}
See the [`kn` documentation]({{ clientdocs() }}){target=_blank} in GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better for accessibility purposes to have link text instead of a URL. Also it is in our style guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should this link go to the 1.1 branch? At the moment, from the preview it looks like it's going to main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, should this link go to the 1.1 branch? At the moment, from the preview it looks like it's going to main.

It should get updated automatically depending on which branch is being built, similar to how the serving/eventing yaml links in the getting started guide are handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see now! Since we're in main it'll go to main.

@snneji
Copy link
Contributor

snneji commented Jan 12, 2022

@rhuss

I think this is a first step in the right direction (so happy to merge this), but eventually, I really would love a tighter and more seamless integration that is easier to consume for the reader.

Let me check if I can consolidate the markdown generation into fewer pages and generate a nice drill-down navigation.

That would be great!

@snneji
Copy link
Contributor

snneji commented Jan 13, 2022

/cherrypick release-1.1

@knative-prow-robot
Copy link
Contributor

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

In response to this:

/cherrypick release-1.1

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.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2022
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: psschwei, snneji

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-robot knative-prow-robot merged commit e9713b1 into knative:main Jan 13, 2022
@knative-prow-robot
Copy link
Contributor

@snneji: #4623 failed to apply on top of branch "release-1.1":

Applying: build kn docs
Using index info to reconstruct a base tree...
M	hack/build.sh
Falling back to patching base and 3-way merge...
Auto-merging hack/build.sh
CONFLICT (content): Merge conflict in hack/build.sh
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 build kn docs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.1

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.

Add kn client commands to the docs
4 participants