-
Notifications
You must be signed in to change notification settings - Fork 30
[CLOUDP-361632] Add functionality to upload release info to GitHub release assets #624
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: master
Are you sure you want to change the base?
Conversation
|
Hi @MaciejKaras , @nammn @mircea-cosbuc @vinilage , thank you, sorry for the confusion. |
| run_on: | ||
| - ubuntu2404-small | ||
| allowed_requesters: ["patch", "github_tag"] | ||
| # depends_on: |
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.
why this is still commented out?
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 will remove this, I have commented this out because I don't want to mistakenly release the images while testing this.
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.
Sure, understand. Let's keep the thread unresolved until this is uncommented.
2f6ea42 to
f5fcd0d
Compare
| run_on: | ||
| - ubuntu2404-small | ||
| allowed_requesters: ["patch", "github_tag"] | ||
| # depends_on: |
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 will remove this, I have commented this out because I don't want to mistakenly release the images while testing this.
| raise e | ||
|
|
||
| def get_manfiest_list_digest(self, image) -> Optional[str]: | ||
| self.pull_image(image) |
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.
Pulling Ops Manager (which is huge) and other images just to get the digest seems like an overkill. Are we sure there is no other way?
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 looked into these ways of figuring out manifest list digest.
docker_cmd.buildx.imagetools.inspect(image_tag)
The python client that we are using doesn't return the manifest list digest, even though the respective CLI returns the required digest. So we can not use this way. I raised an issue upstream.
docker manifest inspect
Does not return the manifest list digest.
docker_cmd.image.inspect(image)
Works well and expects image to be pulled first.
docker inspect
Works well and expects image to be pulled locally.
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 overall process of making the release_info file is taking less than 10 (~6) seconds.
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 other option is to call HTTP API of container registry to figure the information out.
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.
Now that I was thinking about it again, the other option is to not use the python module and just run docker buildx imagetools inspect command directly. The small problem with this approach is, in the other functions we will be using the python package but in one method we will be running docker command directly.
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.
Why not use https://github.com/containers/skopeo to inspect remote images? It's the ideal tool for this case.
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 have used skopeo now to get the manifest list digest.
MCK 1.6.1 Release NotesBug Fixes
|
1. Move mapping field in json to root level 2. Change log lever for logs
1. Use `docker run skopeo inspect` to get digest of manifest list of a container image instead of docker inspect 2. Write unit test
d7108d6 to
f0a8b0f
Compare
| "digest": "sha256:ca4aad523f14d68fccb60256f9ce8909c66ebb5b321ee15e5abf9ac5738947f9" | ||
| }, | ||
| "agent": { | ||
| "repoURL": "quay.io/mongodb/mongodb-agent", |
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.
you've changed repositories from a slice to a string.
The agents are released - as of right now - to 2 different repositories, how would we reflect that now?
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.
Good question! The idea is to share information only about main repository. The other repository we push agent to mongodb-agent-ubi is used for backward compatibility issue. Related comment -> https://github.com/mongodb/mongodb-kubernetes/pull/624/files#r2597674694
| add_image_info( | ||
| release_info_output, | ||
| OPS_MANAGER_IMAGE, | ||
| om_build_info.repositories[0], |
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.
b: We should change the build_info.json instead and instead of having array of repositories there, rather something like this:
"release": {
"sign": true,
"olm-tag": true,
"skip-if-exists": true,
"repository": "quay.io/mongodb/mongodb-agent",
"secondary-repositories": ["quay.io/mongodb/mongodb-agent-ubi"],
"platforms": [
"linux/arm64",
"linux/amd64",
"linux/s390x",
"linux/ppc64le"
]
}cc @nammn
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.
Hi @MaciejKaras,
Even though I agree with the change, I don't think this change should be part of the current PR. If you agree, I have raised this CLOUDP ticket https://jira.mongodb.org/browse/CLOUDP-363995 and we can work on this separately.
If you still think the change should be part of this PR, I can make the change.
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.
It doesn't have and even shouldn't be part of this change. I would rather see this is a prerequisite, separate change. I wouldn't want to have xx_build_info.repositories[0] and hardcoded AGENT_IMAGE_REPOSITORY values in master. My recommendation would be to create a separate PR that splits the repositories field and merge it first (or stack the current PR on top)
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.
ok, so do you think we can merge this PR as it is and then can work on the CLOUDP ticket that I have raised, or do you think we should work on the CLOUDP ticket first and then the merge this PR after refactor?
Summary
As discussed in the base PR, customers have been asking us to provide some details that can be used to figure out the container images that they need to deploy a specific version of MCK and MongoDB instance.
This PR does that. It add the functionality to add a new file called
release_info_<release-version>.jsonto the GitHub release assets that would contain the details about MCK released images, OM, Agent, Search and the mapping between OM and agent's latest versions.This is how the file loos like
https://gist.github.com/viveksinghggits/560e6179a3c7d9b11b0df68f02b15ccc
Proof of Work
Run the patch manually
and make sure the new file is added to the release.
Screen.Recording.2025-12-02.at.23.00.35.mov
Screen.Recording.2025-12-02.at.23.08.27.mov
Checklist
skip-changeloglabel if not needed