Skip to content

Conversation

@grokspawn
Copy link
Contributor

Description

The image pull test requires that the user running it has a permissive image pull policy in place to be successful. However, I run a restrictive policy on my machine, so I get test failures.
Ideally, the unit test should not be dependent on the user's environment, so here's a change which specifies a test-specific, permissive pull policy which will be used instead of the user's environment.

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

Copilot AI review requested due to automatic review settings November 17, 2025 14:59
@grokspawn grokspawn requested a review from a team as a code owner November 17, 2025 14:59
@netlify
Copy link

netlify bot commented Nov 17, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 9a09a4b
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/691dea37396b140008b55048
😎 Deploy Preview https://deploy-preview-2339--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot finished reviewing on behalf of grokspawn November 17, 2025 15:00

return &types.SystemContext{
SystemRegistriesConfPath: registriesConfPath,
SignaturePolicyPath: policyPath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @camilamacedo86!
What's the concern here? Is it that we're dropping a file in t.TempDir() for use in this test scenario?
That's the only difference I see between the approaches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I would love to avoid the temp file generation, this is just a different API than the app uses, and an attempt to avoid writing the overriding signature policy file results in uglier shenanigans.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 19, 2025

Choose a reason for hiding this comment

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

The delta is that we are trying to solve it with the signature if not be possible then we let it go instead of fixed se "default": [{"type": "insecureAcceptAnything"}].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not following you.
Right now this test fails on users' machines with a restrictive signing policy, but the test should not be sensitive to users' environments.
Here creates an insecure signing policy for the scoped tests to be used instead of detecting the users' policies.

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 it is fine

/lgtm

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a test reliability issue by ensuring image pull tests use a permissive signature policy instead of relying on the user's system-level policy. This prevents test failures when developers have restrictive image signing policies configured on their machines.

Key changes:

  • Added creation of an insecureAcceptAnything policy file in the test's temporary directory
  • Configured the SystemContext to use this test-specific policy file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.23%. Comparing base (c06f27f) to head (9a09a4b).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2339      +/-   ##
==========================================
- Coverage   74.30%   74.23%   -0.07%     
==========================================
  Files          91       91              
  Lines        7083     7239     +156     
==========================================
+ Hits         5263     5374     +111     
- Misses       1405     1433      +28     
- Partials      415      432      +17     
Flag Coverage Δ
e2e 44.29% <ø> (-1.36%) ⬇️
experimental-e2e 48.52% <ø> (+0.12%) ⬆️
unit 58.54% <ø> (-0.07%) ⬇️

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: grokspawn <jordan@nimblewidget.com>
@grokspawn grokspawn force-pushed the permissive-pull-policy branch from bf1b6d2 to 9a09a4b Compare November 19, 2025 16:03
@grokspawn
Copy link
Contributor Author

/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2025
@tmshort
Copy link
Contributor

tmshort commented Nov 19, 2025

/approve

@openshift-ci
Copy link

openshift-ci bot commented Nov 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn, tmshort

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 1355ff7 into operator-framework:main Nov 19, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants