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

Add an ability to configure digest evaluation method #79

Merged
merged 4 commits into from
Jun 2, 2021

Conversation

musienko-maxim
Copy link
Collaborator

What does this PR do?

In some specific cases the command can perform too long. For example: when we try to get Index Didgest from a IIB image. For instance: invokation command like skopeo inspect docker://registry-proxy.engineering.redhat.com/rh-osbs/iib:77245 --debug can take about 20-30 minutes.
Screencast:

screencast-nimbus-capture-2021 05 26-18_26_32

As alternative solution we can pull the image locally and get the Didgest with jg. It can decrease a gettin the image didgest. This PR add new target to the Makefile

Is it tested? How?

Has been checked on local crc instance

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I wonder if we can just use podman inspect in all cases (i.e. make register_catalogsource just do what register_catalogsource_for_cpaas does here)

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments but I'll approve ahead of time for when they're fixed

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
IMAGE=$$(podman inspect $(INDEX_IMG) | jq ".[].RepoDigests[0]" -r)

# replace references of catalogsource img with your image
sed -i.bak -e "s|quay.io/wto/web-terminal-operator-index:next|$${IMAGE}|g" ./catalog-source.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can technically extract lines 89-97 to a script and call them from here and from line 75 (since both of those parts are doing essentially the same thing) but it's up to you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote against, but mostly because I also touch these lines in #80 and don't want to fix conflicts 😄

Copy link
Collaborator Author

@musienko-maxim musienko-maxim May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for me it makes sense to keep the previous command with skopeo inspect. Because it does not require pulling of images and potentially it can save space on a file system and avoid to get a long list of images, if a user does not want to pull images.
But if we know that this operation can take a long time - we have alternatives. Actually I used both commands register_catalogsource (with scopeo) and register_catalogsource_for_cpaas (with podman). It just depended on an image which I used. Mostly, if it was not IIB - I used register_catalogsource . Because it worked quickly and did not pull images on the filesystem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@musienko-maxim
well, I don't think it should be a dedicated rule for cpass, but I'm OK with leaving a choice.
Maybe it should be one rule but with an ability to configure impl for digest evaluation, like

GET_DIGEST_WITH='skopeo'
make register_catalogsource

@amisevsk I believe Maxim won't be against if you in your PR just onboard the idea to have an ability evaluate digest with podman

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the GET_DIGEST_WITH env for managing ways of obtaining Didgest (skopeo, podman, docker), remove register_catalogsource_for_cpaas target.

Makefile Outdated Show resolved Hide resolved

# replace references of catalogsource img with your image
sed -i.bak -e "s|quay.io/wto/web-terminal-operator-index:next|$${INDEX_IMG_DIGEST}|g" ./catalog-source.yaml
echo ">>>>>>>catalogsource content:>>>>>>>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to have such output all the time, or it's just what you used for testing?

Copy link
Collaborator Author

@musienko-maxim musienko-maxim Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to save catlog source content, because i need it for the investigating problem with mirroring the IIB images on brew. The problem still actual and resolved partially. After solving all problems we can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could configure that behavior with VERBOSE env var, but that's not a requirement at all. It's small enough to display it by default.

Makefile Outdated Show resolved Hide resolved

# replace references of catalogsource img with your image
sed -i.bak -e "s|quay.io/wto/web-terminal-operator-index:next|$${INDEX_IMG_DIGEST}|g" ./catalog-source.yaml
echo ">>>>>>>catalogsource content:>>>>>>>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could configure that behavior with VERBOSE env var, but that's not a requirement at all. It's small enough to display it by default.

@sleshchenko sleshchenko requested review from amisevsk and JPinkney June 1, 2021 14:25
@sleshchenko
Copy link
Contributor

@JPinkney @amisevsk please review after the latest changes.

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sleshchenko sleshchenko changed the title Add alternative command of register catalog source for CPaaS gating job Add an ability to configure digest evaluation method Jun 2, 2021
@sleshchenko sleshchenko merged commit 96c81fa into redhat-developer:main Jun 2, 2021
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.

4 participants