Skip to content

fix: Removing ttl.sh from PCO on main branch - #302

Merged
tommyd450 merged 3 commits into
mainfrom
tdalton/ttl.shRemoval-main
Aug 6, 2026
Merged

fix: Removing ttl.sh from PCO on main branch#302
tommyd450 merged 3 commits into
mainfrom
tdalton/ttl.shRemoval-main

Conversation

@tommyd450

Copy link
Copy Markdown
Contributor

No description provided.

@tommyd450
tommyd450 requested a review from JasonPowr August 5, 2026 21:22
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 18.33%. Comparing base (c3a2ff3) to head (e3b8fcf).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #302   +/-   ##
=======================================
  Coverage   18.33%   18.33%           
=======================================
  Files           3        3           
  Lines          60       60           
=======================================
  Hits           11       11           
  Misses         49       49           
Flag Coverage Δ
unit 18.33% <ø> (ø)

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

☔ View full report in Codecov by Harness.
📢 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.

@qodo-for-securesign

Copy link
Copy Markdown

PR Summary by Qodo

Fix e2e image publishing by replacing ttl.sh with Quay digest refs

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Publish e2e test images to Quay instead of ttl.sh to unblock main-branch runs.
• Return immutable digest references and use registry auth for reliable pulls.
• Tighten ClusterImagePolicy image glob to match the exact test image reference.
Diagram

graph TD
  A["E2E tests"] --> B["PrepareImage()"] --> C[("Quay.io") ] --> D["Digest ref env"] --> E["CIP template"] --> F["Policy Controller"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Run an in-cluster ephemeral registry for e2e
  • ➕ Eliminates dependency on external registry availability and credentials
  • ➕ Keeps test artifacts fully inside the test environment
  • ➖ Adds setup/teardown complexity and more moving parts to e2e
  • ➖ May not match real-world registry behavior used in production
2. Keep Quay tags and add explicit cleanup instead of expiry labels
  • ➕ Avoids extra HEAD call and dependency on Quay-specific expiry labels
  • ➕ Works uniformly across registries that don’t support expiry metadata
  • ➖ Requires reliable cleanup job/credentials and can leak artifacts on failures
  • ➖ More operational overhead than TTL/expiry metadata
3. Use a dedicated CI registry (GHCR/ECR) with native retention policies
  • ➕ Often better-integrated with CI auth and retention controls
  • ➕ May provide higher reliability/throughput for CI workloads
  • ➖ Registry choice may not be portable across all contributor environments
  • ➖ Requires policy/config changes and secret management updates

Recommendation: The PR’s approach (Quay push + digest pinning) is a good fit for stable, deterministic e2e runs after ttl.sh removal. Digest references reduce flakiness from tag reuse, and the Quay expiry label provides low-maintenance garbage collection. Ensure CI has appropriate credentials available via the default keychain; otherwise pushes/HEAD requests will fail.

Files changed (2) +25 / -6

Bug fix (1) +24 / -5
image.goPush e2e images to Quay with expiry metadata and return digest refs +24/-5

Push e2e images to Quay with expiry metadata and return digest refs

• Replaces ttl.sh tag publishing with pushes to quay.io/securesign/e2e-tests using default keychain auth. Adds a Quay expiry label via config mutation, resolves the pushed image digest, and exports/returns the digest reference for downstream use.

test/utils/image.go

Tests (1) +1 / -1
common_cluster_image_policy.yaml.tplMatch ClusterImagePolicy image glob to the exact test image reference +1/-1

Match ClusterImagePolicy image glob to the exact test image reference

• Updates the policy template to use TEST_IMAGE directly in the images glob, rather than a repository-prefix wildcard. This aligns policy matching with digest-based image references.

test/utils/custom_resources/cluster_image_policies/common_cluster_image_policy.yaml.tpl

@qodo-for-securesign

qodo-for-securesign Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Hardcoded Quay push auth 🐞 Bug ☼ Reliability
Description
PrepareImage now always pushes to the hardcoded repo quay.io/securesign/e2e-tests using
authn.DefaultKeychain and panics on failure, so e2e setup will abort in environments lacking Quay
push credentials/permissions or registry access. This change makes the whole suite sensitive to
external registry auth/availability before any test assertions run.
Code

test/utils/image.go[R43-46]

+	pusher, err := remote.NewPusher(remote.WithAuthFromKeychain(authn.DefaultKeychain))
	if err != nil {
		panic(err.Error())
	}
Relevance

●● Moderate

Hardcoded Quay push/auth may be intentional replacement for ttl.sh; reliability tradeoff unclear
without precedent.

PR-#36
PR-#140

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The image destination is hardcoded to a Quay repo and the push uses DefaultKeychain auth; the
function panics on push errors. Multiple e2e suites call this function in BeforeAll, so any
registry/auth failure aborts the entire suite during setup.

test/utils/image.go[17-62]
test/e2e/policy_controller_e2e_test.go[39-45]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PrepareImage` always pushes a random image to `quay.io/securesign/e2e-tests` using `authn.DefaultKeychain` and `panic`s on any error. This introduces a hard dependency on Quay credentials/permissions and registry reachability for all e2e runs.

### Issue Context
`PrepareImage` is called unconditionally in `BeforeAll` for multiple suites; a push/auth failure prevents all tests from running.

### Fix Focus Areas
- Add an env var (e.g. `E2E_TEST_IMAGE_REPO`) to configure the destination repo instead of hardcoding `quay.io/securesign/e2e-tests`.
- Improve failure mode: if push fails due to auth/permissions, return a clear error message explaining required credentials and how to override via env (instead of a bare `panic(err.Error())`).
- Keep the existing `imageENV` short-circuit, but ensure the default path is usable in CI and local dev with explicit configuration.

- test/utils/image.go[17-62]
- test/e2e/policy_controller_e2e_test.go[39-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Digest lookup ignores context 🐞 Bug ☼ Reliability
Description
PrepareImage accepts a context but calls remote.Head without using that context, so caller
cancellation/deadlines will not govern the digest lookup and can leave tests blocked on registry I/O
longer than intended. This is especially risky because it happens during suite setup.
Code

test/utils/image.go[R53-56]

+	desc, err := remote.Head(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain))
+	if err != nil {
+		panic(err.Error())
+	}
Relevance

●●● Strong

Deterministic reliability fix: use ctx for remote.Head to honor cancellations/timeouts during e2e
setup.

PR-#36
PR-#140

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The function signature includes ctx, and it is used for pusher.Push(ctx, ...), but the new
remote.Head(ref, ...) call is made without any context being passed.

test/utils/image.go[17-58]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PrepareImage(ctx, ...)` uses `ctx` for `pusher.Push`, but the subsequent `remote.Head(...)` does not use the provided context. This means cancellation/timeouts from the caller won’t stop the digest lookup.

### Issue Context
This registry call happens in test setup and can extend suite runtime unexpectedly if the registry is slow/unreachable.

### Fix Focus Areas
- Pass the provided context to the digest lookup (e.g., via a context-capable option for `remote.Head`, if available in the repo’s go-containerregistry version).
- Consider using the same options (auth + context) consistently for all registry operations in `PrepareImage`.

- test/utils/image.go[17-58]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

3. Unused TEST_IMAGE_PREFIX ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The ClusterImagePolicy template no longer references TEST_IMAGE_PREFIX, but tests still compute and
pass it into RenderTemplate, leaving dead configuration that can mislead future edits. This also
keeps ImageRepoPrefix usage alive even though the rendered manifest no longer needs it.
Code

test/utils/custom_resources/cluster_image_policies/common_cluster_image_policy.yaml.tpl[7]

+    - glob: "{{ .TEST_IMAGE }}"
Relevance

●●● Strong

Trivial dead config cleanup; template switched patterns before (e.g., TEST_IMAGE_PREFIX added
earlier).

PR-#118
PR-#140

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The template now renders spec.images[0].glob from .TEST_IMAGE (and contains no
.TEST_IMAGE_PREFIX reference), but the test suite still passes TEST_IMAGE_PREFIX into the
template data map when rendering the manifest.

test/utils/custom_resources/cluster_image_policies/common_cluster_image_policy.yaml.tpl[5-15]
test/e2e/policy_controller_e2e_test.go[81-92]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The ClusterImagePolicy template switched from using `TEST_IMAGE_PREFIX` to using `TEST_IMAGE`, but callers still pass `TEST_IMAGE_PREFIX` into the template data map.

### Issue Context
This doesn’t change runtime behavior, but it creates dead configuration and suggests the template still needs a prefix when it no longer does.

### Fix Focus Areas
- Remove `TEST_IMAGE_PREFIX` from the data maps passed to `RenderTemplate`.
- If `ImageRepoPrefix` becomes unused after cleanup, remove it (or keep it only if other templates still need it).

- test/utils/custom_resources/cluster_image_policies/common_cluster_image_policy.yaml.tpl[5-15]
- test/e2e/policy_controller_e2e_test.go[81-92]
- test/e2e/update_e2e_test.go[127-137]
- test/e2e_upgrade/upgrade_test.go[197-208]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread test/utils/image.go
Comment on lines +43 to 46
pusher, err := remote.NewPusher(remote.WithAuthFromKeychain(authn.DefaultKeychain))
if err != nil {
panic(err.Error())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Hardcoded quay push auth 🐞 Bug ☼ Reliability

PrepareImage now always pushes to the hardcoded repo quay.io/securesign/e2e-tests using
authn.DefaultKeychain and panics on failure, so e2e setup will abort in environments lacking Quay
push credentials/permissions or registry access. This change makes the whole suite sensitive to
external registry auth/availability before any test assertions run.
Agent Prompt
### Issue description
`PrepareImage` always pushes a random image to `quay.io/securesign/e2e-tests` using `authn.DefaultKeychain` and `panic`s on any error. This introduces a hard dependency on Quay credentials/permissions and registry reachability for all e2e runs.

### Issue Context
`PrepareImage` is called unconditionally in `BeforeAll` for multiple suites; a push/auth failure prevents all tests from running.

### Fix Focus Areas
- Add an env var (e.g. `E2E_TEST_IMAGE_REPO`) to configure the destination repo instead of hardcoding `quay.io/securesign/e2e-tests`.
- Improve failure mode: if push fails due to auth/permissions, return a clear error message explaining required credentials and how to override via env (instead of a bare `panic(err.Error())`).
- Keep the existing `imageENV` short-circuit, but ensure the default path is usable in CI and local dev with explicit configuration.

- test/utils/image.go[17-62]
- test/e2e/policy_controller_e2e_test.go[39-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@tommyd450

Copy link
Copy Markdown
Contributor Author

/retest

JasonPowr
JasonPowr previously approved these changes Aug 6, 2026
spec:
images:
- glob: "{{ .TEST_IMAGE_PREFIX }}**"
- glob: "{{ .TEST_IMAGE }}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

commonTestImage can be empty here because it's only set in the "common installation" tests, and Ginkgo doesn't guarantee which test group runs first. Revert the glob to "{{ .TEST_IMAGE_PREFIX }}**" so an empty value still produces a valid glob.

@tommyd450
tommyd450 merged commit ef8ab13 into main Aug 6, 2026
1 of 6 checks passed
@tommyd450
tommyd450 deleted the tdalton/ttl.shRemoval-main branch August 6, 2026 10:02
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