-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(docs): add workflow to generate documentation on PR merge #12681
base: master
Are you sure you want to change the base?
feat(docs): add workflow to generate documentation on PR merge #12681
Conversation
Co-authored-by: Rod Vagg <[email protected]>
.github/workflows/check.yml
Outdated
@@ -26,21 +26,19 @@ jobs: | |||
steps: | |||
- uses: actions/checkout@v4 | |||
with: | |||
submodules: 'recursive' | |||
submodules: "recursive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should check your editor and remove these auto-fix rules that you have, it's generally not good manners to go editing things unrelated to what you're intending to change without good reason to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@virajbhartiya Also single quote and double quote have specific meaning in YAML, similar to bash. Here, single quote is appropriate because there are no special characters expected/acceptable in recursive
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, my linter was automatically replacing the quotes, fixed it
@virajbhartiya can you resolve the conflict with |
Hey @rvagg, I have resolved the conflicts in the PR |
@rvagg any suggestions on why is the workflow failing? |
Failing because you need to install system dependencies for the build in check.yml, you'll see: - uses: ./.github/actions/install-system-dependencies
- uses: ./.github/actions/install-go
- uses: ./.github/actions/make-deps these are required to perform the actions you need, so you'll have to do it again in your new job but, having said that, it's not cheap, so maybe this should all be in a single job so that's all done once and it's just the docsgen-cli bit that's done separately. So how about this:
I'm still not convinced this is a great idea, it might get in the way. So we'll need to have a collective chat with others before we proceed. |
Hey @rvagg thanks for the review, I have currently modified it accordingly, renamed check.yml to check-and-gen.yml and added it in the same file. We can discuss regarding this on slack wtih other how to go ahead with this PR |
Related Issues
Closes #12233
Proposed Changes
Add a workflow that run
make docsgen-cli
whenever the PR is mergedChecklist