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

stop building "scorecard-storage", "scorecard-untar" images #6422

Conversation

acornett21
Copy link
Contributor

Description of the change:
Remove "scorecard-storage", "scorecard-untar" from being built during the release process. I think this is the only place to change, though I am unsure if this is all the places.

Motivation for the change:
In order for scorecard tests to work in DCI and disconnected environments, images need to be referenced by sha and not a tag, this would stop these two testing images from getting rebuilt on release, since the rarely change.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@acornett21 acornett21 temporarily deployed to deploy May 8, 2023 19:17 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 8, 2023 19:17 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 8, 2023 19:17 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 8, 2023 19:17 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 8, 2023 19:17 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 8, 2023 19:17 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 8, 2023 19:17 — with GitHub Actions Inactive
Copy link
Contributor

@tkrishtop tkrishtop left a comment

Choose a reason for hiding this comment

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

Hi @acornett21, thank you for starting the PR.

It would also be nice to remove scorecard-untar and scorecard-storage from the Makefile to avoid rebuilding the images:

IMAGE_TARGET_LIST = operator-sdk helm-operator ansible-operator ansible-operator-2.11-preview scorecard-test scorecard-test-kuttl scorecard-untar scorecard-storage

We could also fix the digests at the same time by using the current digests for scorecard-storage and scorecard-untar:

quay.io/operator-framework/scorecard-storage@sha256:a3bfda71281393c7794cabdd39c563fb050d3020fd0b642ea164646bdd39a0e2

quay.io/operator-framework/scorecard-untar@sha256:2e728c5e67a7f4dec0df157a322dd5671212e8ae60f69137463bd4fdfbff8747

Here is our previous PR for changing the digests. We need more or less the same here.

@acornett21
Copy link
Contributor Author

@tkrishtop I wasn't sure if the maintainers also wanted the makefile changed or not, was hoping for their feedback on this. I think the sha changes should be a separate PR for two reasons, 1) to not confuse the intent of this PR, 2) incase a release is cut before this gets merged and the shas change. So the intent is the sha changes will be in a separate PR. Hope this makes sense.

@tkrishtop
Copy link
Contributor

@tkrishtop I wasn't sure if the maintainers also wanted the makefile changed or not, was hoping for their feedback on this. I think the sha changes should be a separate PR for two reasons, 1) to not confuse the intent of this PR, 2) incase a release is cut before this gets merged and the shas change. So the intent is the sha changes will be in a separate PR. Hope this makes sense.

  1. @acornett21 agree, let's wait for @everettraven approval for Makefile.

  2. The shas were already changed while cutting off the 1.28.1 release. The digests, currently used in the code, do not exist anymore, and the master branch is broken

$ podman pull quay.io/operator-framework/scorecard-storage@sha256:f7bd62664a0b91034acb977a8bb4ebb76bc98a6e8bdb943eb84c8e364828f056
Trying to pull quay.io/operator-framework/scorecard-storage@sha256:f7bd62664a0b91034acb977a8bb4ebb76bc98a6e8bdb943eb84c8e364828f056...
Error: initializing source docker://quay.io/operator-framework/scorecard-storage@sha256:f7bd62664a0b91034acb977a8bb4ebb76bc98a6e8bdb943eb84c8e364828f056: reading manifest sha256:f7bd62664a0b91034acb977a8bb4ebb76bc98a6e8bdb943eb84c8e364828f056 in quay.io/operator-framework/scorecard-storage: manifest unknown

Since we have nothing to lose, I proposed to fix everything at once, but please do as you think better.

@tkrishtop
Copy link
Contributor

Hi @varshaprasad96, can you please check the question about the Makefile above? I usually ask Bryce to help us out but he is off this week.

@everettraven
Copy link
Contributor

I wasn't sure if the maintainers also wanted the makefile changed or not, was hoping for their feedback on this. I think the sha changes should be a separate PR for two reasons, 1) to not confuse the intent of this PR, 2) incase a release is cut before this gets merged and the shas change. So the intent is the sha changes will be in a separate PR. Hope this makes sense.

@acornett21 @tkrishtop I think it is fine to update the Makefile as part of this PR although I don't think the Makefile has a direct impact on the release process in regards to building and pushing the images. Just to make sure I am following along though, I think the only change in the Makefile would be here:

IMAGE_TARGET_LIST = operator-sdk helm-operator ansible-operator ansible-operator-2.11-preview scorecard-test scorecard-test-kuttl scorecard-untar scorecard-storage

+1 on the sha changes being in a separate PR

Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

@acornett21 the Makefile can be updated as part of this PR if you would like but I don't think it should block this PR from making it in. From what I recall, the Makefile target for building the images is just used for local building and isn't a part of the release pipeline.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2023
…e since these images need to be referenced by a sha, and rarely change.

Signed-off-by: Adam D. Cornett <adc@redhat.com>
@acornett21 acornett21 force-pushed the stop_building_scorecard_images branch from a4256ee to 1df3b41 Compare May 15, 2023 14:07
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 15, 2023
@acornett21 acornett21 temporarily deployed to deploy May 15, 2023 14:08 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 15, 2023 14:08 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 15, 2023 14:08 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 15, 2023 14:08 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 15, 2023 14:08 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 15, 2023 14:08 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy May 15, 2023 14:08 — with GitHub Actions Inactive
@acornett21
Copy link
Contributor Author

@everettraven I updated the makefile as well.

@varshaprasad96 varshaprasad96 added this to the v1.29.0 milestone May 15, 2023
@acornett21 acornett21 mentioned this pull request May 15, 2023
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2023
@everettraven
Copy link
Contributor

Merging this despite test failures since this PR is part of helping fix these failures longer term. Thanks again @acornett21 for creating this PR!

@everettraven everettraven merged commit 49b7f00 into operator-framework:master May 16, 2023
23 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants