-
Notifications
You must be signed in to change notification settings - Fork 845
ci: confirm expected autogenerated reference exists #1759
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1759 +/- ##
=======================================
Coverage 84.95% 84.95%
=======================================
Files 113 113
Lines 12905 12905
=======================================
Hits 10964 10964
Misses 1941 1941 ☔ View full report in Codecov by Sentry. |
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.
✅ Love love love this! I'm so excited to see this run. If it works well, perhaps we add it to the Node SDK too?
@mwbrooks I share your excitement! 📚 ✨ An above check shows that the current changes of
I'll add these changes to this PR for continued testing and polish 📠 |
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.
🗣️ note: We're hiding generated documentation with this change!
-U -r requirements/optional.txt | ||
|
||
echo "Generating code ..." && python scripts/codegen.py --path . | ||
echo "Generating docs ..." && CI=true bash scripts/generate_api_docs.sh |
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.
🔍 question: I'm unsure if this is alright to add to this file. At the moment this script might create uncommitted changes without causing this script to error.
🗣️ ramble: I can imagine another environment perhaps being used to run the diff
commands that exist in the GitHub workflows at the moment, but I found this implementation to be alright for remembering to generate these changes alongside the PR requirements to run tests!
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.
I fear that including the docs generation here may lead to more work for code reviewers 😅 Since we ask developers to run scripts/run_validation.sh
before opening PRs, this might double the average length of our PRs.
This may add more complexity for reviewers review code changes and may make it harder for maintainers to resolve merge conflicts when collaborating on similar features.
Updating the generated docs as a release step has been working well, maybe we could clean up the release instructions to make it more clear. Let me know what you think
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.
@WilliamBergamin Thank you so much for sharing your findings with releases so far 🙏 ✨
I agree the generated reference causes a significant diff
but we're hiding those from the Files changed tab of a PR with a new ".gitattributes" file. I don't believe these pages require review at all if the CI check passes to confirm these match the expected generated files.
This change overall was motivated in reducing maintenance across development - reference pages of development branches were either missing notes or included new changes from commits since the last release, which was confusing in previews I find.
Merge conflicts hadn't yet happened for me in recent changes either but I do think regenerating reference from the related code might be a suitable fix. I'm hoping to discuss keeping docs with the related commit more since we can now release docs according to a particular release tag instead of main
🤖
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.
This does all make sense 💯
I am still hesitant, there has been situation where I've notices doc generation issue when cutting releases. Those were found when reviewing the diff
of the release PR. Configuring the ".gitattributes" file so that the diff
around generated references is always hidden would prevent reviewers from catching errors around the docs generation. We would be blindly trusting the generation script 😅
IIRC at some point the path of the generation script was changes by mistake, this caused the generation script to generate the docs in a new directory while leaving the old docs unchanged, this was caught in the release PR through the diff
It may still be useful to allow reviewers to view the diff
of the generated docs at some critical points in our development cycle
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.
thanks for putting this together 💯
I left a comment on including the docs generation in the validation script, let me know if you want to discuss more on this
-U -r requirements/optional.txt | ||
|
||
echo "Generating code ..." && python scripts/codegen.py --path . | ||
echo "Generating docs ..." && CI=true bash scripts/generate_api_docs.sh |
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.
I fear that including the docs generation here may lead to more work for code reviewers 😅 Since we ask developers to run scripts/run_validation.sh
before opening PRs, this might double the average length of our PRs.
This may add more complexity for reviewers review code changes and may make it harder for maintainers to resolve merge conflicts when collaborating on similar features.
Updating the generated docs as a release step has been working well, maybe we could clean up the release instructions to make it more clear. Let me know what you think
Summary
This PR adds a check that confirms the expected autogenerated reference exists for a commit to confirm build previews compile and to reduce the overhead of a release 📚
For slackapi/slack-health-score#116 and investigation toward #1752!
Testing
Category
/docs
(Documents)tests
/integration_tests
(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes.