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

feat(docs): add workflow to generate documentation on PR merge #12681

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

virajbhartiya
Copy link
Member

@virajbhartiya virajbhartiya commented Nov 7, 2024

Related Issues

Closes #12233

Proposed Changes

Add a workflow that run make docsgen-cli whenever the PR is merged

Checklist

@@ -26,21 +26,19 @@ jobs:
steps:
- uses: actions/checkout@v4
with:
submodules: 'recursive'
submodules: "recursive"
Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

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

@rvagg
Copy link
Member

rvagg commented Nov 7, 2024

@virajbhartiya can you resolve the conflict with master here please? You'll see that your " substitutions are getting in the way, so resolving those would be good.

@virajbhartiya
Copy link
Member Author

Hey @rvagg, I have resolved the conflicts in the PR

@virajbhartiya
Copy link
Member Author

@rvagg any suggestions on why is the workflow failing?

@rvagg
Copy link
Member

rvagg commented Nov 18, 2024

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:

  1. rename check.yml to check-and-gen.yml
  2. add a docsgen-cli section to it with your new stuff, so that happens in the same flow

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.

@virajbhartiya
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 Awaiting review
Development

Successfully merging this pull request may close these issues.

Run make docsgen-cli on merge
4 participants