Skip to content

fix(ci): allow check collector/scanner failure#14825

Closed
BradLugo wants to merge 1 commit intomasterfrom
adjust-check-images
Closed

fix(ci): allow check collector/scanner failure#14825
BradLugo wants to merge 1 commit intomasterfrom
adjust-check-images

Conversation

@BradLugo
Copy link
Contributor

@BradLugo BradLugo commented Apr 1, 2025

Description

Our Build / check-collector-images-exist and Build / check-scanner-images-exist cause the release workflows to fail if they fail. See previous runs from this action for examples: https://github.com/stackrox/stackrox/actions/runs/14172544638

I propose we use continue-on-error for these jobs, which I think will give us the ability to continue on with the release pipeline, while indicating failures in PRs.

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

N/A

How I validated my change

I haven't yet.

@rhacs-bot
Copy link
Contributor

Images are ready for the commit at de5993d.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-354-gde5993d1fb.

@codecov
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.88%. Comparing base (bcd866a) to head (de5993d).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14825      +/-   ##
==========================================
- Coverage   48.88%   48.88%   -0.01%     
==========================================
  Files        2547     2547              
  Lines      186906   186906              
==========================================
- Hits        91361    91360       -1     
  Misses      88313    88313              
- Partials     7232     7233       +1     
Flag Coverage Δ
go-unit-tests 48.88% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

echo "collector-version=$(make --quiet --no-print-directory collector-tag)" >> "${GITHUB_OUTPUT}"

- name: Check image exists
continue-on-error: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good idea, but I have concerns that also default images (without -fast suffix) will be ignored, and that will cause problems in other places.

What do you think about separating the "default" images check from the fast images check? (in short, unwrap tag_suffix: ["", "-fast"] in two jobs) - or have conditional failure handling for -fast tags.

Copy link
Contributor

@msugakov msugakov Apr 1, 2025

Choose a reason for hiding this comment

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

If you look at the git history, the default image check was introduced in #13502. There was no reason for it to exist before Konflux was the thing. There is no need for it to check GHA-built images presence.

The reason it seemingly does so, i.e. check GHA tags with "" suffixes is because there wasn't enough clarity whether Konflux-built ScannerV2 and Collector images for release are going to have no suffixes or would come with -fast in tags.
There's still unclarity, but it seems more that ScannerV2 and Collector will remain -fast suffixed (as originally built in their repos) even in release Konflux builds. Therefore we can drop "" from checking.

There are couple other things that bother me even though I reviewed and approved the original PR.
I don't see a good reason to have these checks required by build-and-push-main because the one does not depend on any artifacts from check-*-images-exist because they produce no artifacts. The protection also exists in a different place, check-*-images-exist are required for master merges.
Secondly, I don't see why these checks have to be in the build.yaml.

With all that, I think, it's quickest to create an alternative PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Draft in #14827

@BradLugo
Copy link
Contributor Author

BradLugo commented Apr 3, 2025

Closing in favor of #14827.

@BradLugo BradLugo closed this Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants