Skip to content
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

Closed
wants to merge 9 commits into from
Closed

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Feb 28, 2019

@ruffsl starting from release_hash in #204 this merges master into itself, adds a python script to update the InRelease file hashes, and updates the hashes. If it looks ok to you feel free to merge this into release_hash.

The script does not change the date in the echo'd string. Maybe it could echo the hash instead?

@sloretz sloretz requested a review from ruffsl February 28, 2019 00:36
@sloretz sloretz changed the title Release hash shane Release hash script and update hashes Feb 28, 2019
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
print(url)
print("\t" + hash_output)

paths = {
Copy link
Member

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.

osrf/docker_templates#45

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

@sloretz sloretz Mar 19, 2019

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

Copy link
Member

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.

pr_docker_library.py Outdated Show resolved Hide resolved


def update_library_definition_fork(name):
# Create branch on fork that is up to date with upstream
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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

@mikaelarguedas
Copy link
Contributor

mikaelarguedas commented Sep 27, 2020

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 ?

@sloretz sloretz closed this Sep 29, 2020
@sloretz sloretz deleted the release_hash_shane branch September 29, 2020 15:47
@ruffsl
Copy link
Member

ruffsl commented Oct 10, 2020

@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.
#204 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants