Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ci: Automated Y-Stream Releases #190
base: main
Are you sure you want to change the base?
ci: Automated Y-Stream Releases #190
Changes from all commits
2e6a5f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Where is this requirement to cap coming from? Aren't we asked to not cap?
Without this, steps 3-5 are no longer necessary.
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 have been capping certain dependencies in the core repo since October 2024: https://github.com/instructlab/instructlab/pulls?q=is%3Apr+%22deps%3A+cap%22+is%3Aclosed+
To elaborate, we have been capping our own InstructLab library dependencies to ensure that if we create a new release from the core repo, that new release won't automatically consume potential breaking changes from one of our own libraries.
If we don't cap certain dependencies, then we'll need to address those breaking changes in one or more pull requests and follow up by publishing a Z-stream release so that end users can consume the fixes. So I think for end users in particular, it can be extremely frustrating to pull the latest InstructLab release from 1-7 days ago, only to find out that the InstructLab release doesn't work because it's pulling an incompatible package.
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.
Also, to be clear, the "automatic capping" feature is not required to be used by anybody. Maintainers can ignore the feature if desired. And since the automation described here will create a pull request that someone has to manually approve and review, the dependency capping changes will not be merged automatically. 😃
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 feel the commas and "and" aren't really necessary in a numbered list
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 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 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.
Would we want to make this also to be available to run outside of this cron schedule for any reason, by exposing the inputs for the workflow ?
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.
Yes, as mentioned near the bottom of the document, there is an option to add your own trigger conditions, like
workflow_dispatch
. When usingworkflow_dispatch
, users can manually kick off a release process.The trigger conditions is defined at the Git workflow level, but not at the
create-automated-release
level.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.
See above. Capping is not something we should automate. It's an exceptional situation, not a matter of course.
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.
@courtneypacheco Would this
automated-release-config.yml
file be pery
stream release ? Is there a way we could add a checkpoint to verify in the beginning of the workflow to make sure this file exists and if it does, make sure it exists with the necessary details and format ? We could skip the secondary validation, if the file doesn't exist, which seems to be a possibility based on your workflow diagram.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.
Also would it be a good idea to version this file within the
y
stream release, if things change on us ? In other words, do we want this file to be immutable once the release process kicks off.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 now, per y-stream release.
I definitely do plan on adding a check to see if the file exists. :) If it the file doesn't exist, then the code will resort to built-in default values where applicable.
Since dependency capping is the only "configurable" component right now, the default value for the configurable list of components would be an empty list.
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.
Thank you.
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.
Suggestion here, lmk what you think
You could have similar fields
equals
orless_than
when applicableThere 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.