-
Notifications
You must be signed in to change notification settings - Fork 551
Adjust release process after quickstart changes #4104
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: develop
Are you sure you want to change the base?
Conversation
Documentation Link Check Results❌ Absolute links check failed |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
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 am quite confused by the current state of this workflow right now. Maybe I am missing something, but this definitely needs to be tested with a dummy release branch before being merged in.
| --platform linux/amd64 \ | ||
| --build-arg BASE_IMAGE=$$USERNAME/prepare-release:base-${_ZENML_NEW_VERSION} \ | ||
| --build-arg CLOUD_PROVIDER=gcp \ | ||
| --build-arg ZENML_BRANCH=${_ZENML_BRANCH} \ |
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 am confused about all this. Maybe you can clear it for me. I see now that we don't even build any quickstart images for the release (which I like). But if that's the case, why do we even need to build them now for testing the release? Isn't ok for us to use the base image to test?
Previously, different quickstarts required a different "pre-specified" list of requirements to run the quickstart. As far as I can see, this is no longer a concern of the new one.
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.
These image still installed the stack requirements in there, but I don't even understand why that was necessary back then. I removed all of that now though.
| fi | ||
| done | ||
| git add examples/quickstart | ||
| - name: Regenerate quickstart cloud requirements |
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.
As far as I can see this used to generate examples/quickstart/requirements_{aws,azure,gcp}.txt files for the quickstart testing script. Since this is now disabled (which was the right call I think), wouldn't the qs_run_release_flow fail?
It has a check:
...
REQS="requirements_${CLOUD}.txt"
...
if [[ ! -f "${REQS}" ]]; then
echo "Error: Requirements file not found: ${REQS}" >&2
exit 1
fi
...
``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.
Same script also looks for config files that do not exist.
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.
The installation seems a bit weird as well. The runner starts, checks out to the branch, already install ZenML once, passes the branch name to the test script, the test script manually replaces the hard-coded zenml requirement (which is the only requirement), pip installs zenml throught the requirements.txt file again and only then runs the example. This seems like a very complicated and redundant process.
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.
Yep, this whole thing has been a mess. I removed as much of it as I could now.
ae74708 to
831a33a
Compare
Describe changes
The new quickstart doesn't include running on a cloud stack anymore. This PR removes any files/code that was still building Docker images for that purpose.
Note: We still run the quickstart on cloud stacks as part of the release process.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes