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

fix: add check during image discovery to make sure images are valid #3234

Merged
merged 8 commits into from
Feb 19, 2025

Conversation

a1994sc
Copy link
Contributor

@a1994sc a1994sc commented Nov 12, 2024

Description

When using zarf to discover images with the Ironbank chart kyverno-policies, the restrict-image-registries policy, (simple version included in tests), has an entry of:

  • registry.dso.mil/*

This is a false positive and cases cosign looks up to fail as that is not a valid image.

Checklist before merging

Copy link

netlify bot commented Nov 12, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit f811a85
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/6733c4caa3301a0008950240

Copy link

netlify bot commented Nov 12, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 0b2f368
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/67b21f3682261b0008eac692

@a1994sc a1994sc marked this pull request as ready for review November 12, 2024 21:18
@a1994sc a1994sc requested review from a team as code owners November 12, 2024 21:18
@a1994sc
Copy link
Contributor Author

a1994sc commented Nov 20, 2024

Related to #3253

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/pkg/packager/prepare.go 53.84% <100.00%> (+0.38%) ⬆️
src/pkg/packager/regex.go 100.00% <100.00%> (ø)

@a1994sc a1994sc force-pushed the fix-regex-for-dev-find branch from 06ee1fd to b2b355c Compare February 16, 2025 17:16
@a1994sc
Copy link
Contributor Author

a1994sc commented Feb 18, 2025

Any reason those two test are stuck?

@brandtkeller
Copy link
Contributor

Any reason those two test are stuck?

GitHub reports "unexpected error" - so I'm going to give those a retry real quick.

@a1994sc
Copy link
Contributor Author

a1994sc commented Feb 18, 2025

Any reason those two test are stuck?

GitHub reports "unexpected error" - so I'm going to give those a retry real quick.

Ah, thank you very much!!

Copy link
Contributor

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

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

This definitely improves the baseline. I'll let another maintainer weigh-in if there is any patterns or constraints we want to follow with regards to the regex logic - otherwise LGTM.

@a1994sc
Copy link
Contributor Author

a1994sc commented Feb 18, 2025

Thanks!! I was a little worried that because you all are migrating to package2 it might have needed to be put on hold. Though very happy to help an awesome project!

Copy link
Contributor

@mkcp mkcp left a comment

Choose a reason for hiding this comment

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

LGTM. Good test case and defense for the logic. Matching the license and incl. attribution makes accepting this easy. Nice job

@a1994sc
Copy link
Contributor Author

a1994sc commented Feb 19, 2025

I would be happy to help with the effort to port over to the package2 implementation, whenever that works starts

@brandtkeller brandtkeller added this pull request to the merge queue Feb 19, 2025
Merged via the queue into zarf-dev:main with commit a8377be Feb 19, 2025
26 checks passed
@a1994sc
Copy link
Contributor Author

a1994sc commented Feb 19, 2025

Thanks Team!!

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.

3 participants