-
Notifications
You must be signed in to change notification settings - Fork 170
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
Release hash script and update hashes #230
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
update_cache_busting_hashes.py
Outdated
print(url) | ||
print("\t" + hash_output) | ||
|
||
paths = { |
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 enumeration of paths doesn't seem very maintaintable. I'd recommend we keep with template library updating the files, and simply checking if the diff of the file has changed compared to the commit history.
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.
Ah, I missed that PR. I should have figured those sections were already created by a script.
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'd recommend we keep with template library updating the files, and simply checking if the diff of the file has changed compared to the commit history.
I'm on board with osrf/docker_templates
updating the files, but I don't understand what the second half of the sentence means. What checks the diff of what file compared to what commit history?
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.
deleted this file in 815e3ff
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.
What checks the diff of what file compared to what commit history?
The script (that runs after the maintainer merges the last relevant PR and the approved this CI job) should first check if the PR itself results in changes to the manifest for the respective dockerhub repo. If PR changes the manifest file, it should then check if the manifest in osrf/docker_images/blob/master/<repo>/<repo>
is out of data w.r.t docker-library/official-images/blob/master/library/<repo>
. If the upstream manifest is out of sync, in should then check if no existing PR is open for the respective repo (perhaps checking the branch on our ros-infrastructure fork). If no existing PR exists, then it should push to the branch on our fork and open a templated PR, @ pinging us as maintainers.
|
||
|
||
def update_library_definition_fork(name): | ||
# Create branch on fork that is up to date with upstream |
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.
Is this pushing the manifest (post merged) copied from osrf/docker_images to the FORK_LIBRARY_REPO_URL
or only that from the checked out PR? To consolidate all the release changes for a given docker repo to one at a time, I think pushing the merged state of the repo manifest would be less overwhelming upstream that PRing each change. E.g. we'd only approve this automated upstream PR for the last merged PR to a given docker repo to batch release changes. This is similar to what we do now manually.
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 don't understand the question. Here is an example PR created by this script: ros-infrastructure/official-images#8 . Does the diff answer it?
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.
Your pr is a multiple merged distro changes, yet local PRs by the @osrf-docker-builder separate each distro PR, so the maintainer should be prudent in waiting for all the changes targeting a given repo manifest to be merged before upstreaming the final updated manifest. To avoid opening to many PR, they are batched into one periodic PR for that respective dockerhub repo.
Example PR:
#233
#234
#235
#236
#231
Upstream PR:
docker-library/official-images#5573
I believe this can be closed in favor of osrf/docker_templates#90 @ruffsl If there anything from the automatic PR work on this branch we want to keep in a new PR ? |
@mikaelarguedas , Looks like the librarians aren't a fan of our sha lable tricks. Should we revert that and go back to this instead? The docker librarians seemed to like this approach as it would block un-reproducible builds by acting upon the discrepancies. |
@ruffsl starting from
release_hash
in #204 this merges master into itself, adds a python script to update theInRelease
file hashes, and updates the hashes. If it looks ok to you feel free to merge this intorelease_hash
.The script does not change the date in the echo'd string. Maybe it could echo the hash instead?