-
Notifications
You must be signed in to change notification settings - Fork 52
Add workflow to create and save versioned references #332
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
Conversation
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.
5 files reviewed, 5 comments
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
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.
3 files reviewed, 2 comments
| if: steps.changes.outputs.changed == 'true' | ||
| with: | ||
| commit_message: "Update generated references" | ||
| file_pattern: references/ No newline at end of file |
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.
style: Missing newline at end of file.
| file_pattern: references/ | |
| file_pattern: references/ |
| output_file = os.path.join( | ||
| str(OUTPUT_CONFIG["output_dir"]), str(OUTPUT_CONFIG["filename"]) | ||
| ) | ||
| output_file_latest = os.path.join( | ||
| str(OUTPUT_CONFIG["output_dir"]), str(OUTPUT_CONFIG["filename_latest"]) | ||
| ) |
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.
logic: The script writes to ./references/ directory but doesn't ensure it exists first. This could cause a FileNotFoundError if the directory doesn't exist.
| output_file = os.path.join( | |
| str(OUTPUT_CONFIG["output_dir"]), str(OUTPUT_CONFIG["filename"]) | |
| ) | |
| output_file_latest = os.path.join( | |
| str(OUTPUT_CONFIG["output_dir"]), str(OUTPUT_CONFIG["filename_latest"]) | |
| ) | |
| output_file = os.path.join( | |
| str(OUTPUT_CONFIG["output_dir"]), str(OUTPUT_CONFIG["filename"]) | |
| ) | |
| output_file_latest = os.path.join( | |
| str(OUTPUT_CONFIG["output_dir"]), str(OUTPUT_CONFIG["filename_latest"]) | |
| ) | |
| # Ensure output directory exists | |
| os.makedirs(str(OUTPUT_CONFIG["output_dir"]), exist_ok=True) |
rafaeelaudibert
left a comment
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.
There's a tiny bug here since the workflow is running more often than needed
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.
We don't have this as a requirement in our SDKs, but it'd be better if you set the actions to a specific commit hash rather than to a version like you're doing here. GitHub's ones are fine, but the astral and stefanzweifel ones could be easily compromised/abused. You can look at our main repo to see how that works.
Just a nice to have
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.
Wonderful, I go back to a hash lol. I also agree
I used a hash but greptile was like nono :finger wag:
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| workflow_dispatch: |
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.
Not all pushes to master release a new version. Can we add something here to restrict this from running at all times?
Or, maybe something to detect we've already generated docs for that version?
My fear is the following:
- Release V1.0.0
- This workflow generates spec for v1.0.0 and adds in the commit
- Merges a new feature on master but does NOT release it as a new feature
- This workflow generates spec for v1.0.0 and adds in the commit
⚠️ ⚠️ ⚠️ ⚠️ ⚠️ WRONG
We'd have overwritten the docs for v1.0.0 incorrectly
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 think we should still bump latest right (meant to be head of the branch) or do we not bother?
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.
Hmm, I don't think so. We could update something called edge or master, but latest should probably always be the "latest that was actually released".
|
| release: | ||
| types: [published] |
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've double checked and we do publish releases here in Github, so lgtm!
We want automatically updated, versioned, and persisted references for all SDKs.
See related RFC
This PR does these things:
See this run on my fork for the workflow
Feel free to bring up suggestions/objections to this approach, but let's keep the discussion in the related RFC for anything that's not repo specific <3
Looking forward to y'all's wisdom and help!