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

Pin conformance tests to main #3978

Closed
wants to merge 1 commit into from

Conversation

codysoyland
Copy link
Member

@codysoyland codysoyland commented Dec 17, 2024

Summary

This PR changes the version of the conformance tests to be pinned to main instead of the latest release.

Previous discussion in #3965 was unresolved, so it was merged with the conformance tests action referencing to latest release instead of main as a compromise.

There was a slight philosophical disagreement in the previous PR: It was opened referencing main, but there are some reasons you would not want to pin to main:

  • Dependabot updates should keep it up to date anyway (at least up to the latest release), however this seems to have failed recently.
  • Changes to main conformance tests could block unrelated changes in cosign.
  • Referencing a main branch is not repeatable so could confuse cosign developers if suddenly the conformance tests were broken as a result of upstream changes.

However there are good counterpoints:

  • Conformance tests recently had a ~9 month gap between releases, so even with Dependabot, there could be long periods of drift.
  • It's important for conformance tests to be fixed ASAP as they could represent security flaws.

Generally I'm of the opinion that Actions should be pinned to specific version hashes for strong repeatability, however I can understand in this case that the benefit of catching conformance failures early may outweigh the drawbacks, especially after noticing the recent gap between conformance releases.

As an alternative to this PR, we could instead add a second conformance test run that runs the main branch (so there would be two versions of the conformance tests run, one on the latest release and one on main). This way, we can use the most recent released version as a branch protection, and use the main branch one as an informative check that does not block PRs. The main one could even run on a nightly schedule and create an issue if there is a failure. I'm open to this option as a "stable" alternative that would give us much of the same benefit, but I'm okay with either option.

Release Note

Documentation

Signed-off-by: Cody Soyland <[email protected]>
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.46%. Comparing base (2ef6022) to head (24c1ef6).
Report is 258 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3978      +/-   ##
==========================================
- Coverage   40.10%   36.46%   -3.65%     
==========================================
  Files         155      209      +54     
  Lines       10044    13312    +3268     
==========================================
+ Hits         4028     4854     +826     
- Misses       5530     7839    +2309     
- Partials      486      619     +133     

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

@bobcallaway
Copy link
Member

As an alternative to this PR, we could instead add a second conformance test run that runs the main branch (so there would be two versions of the conformance tests run, one on the latest release and one on main). This way, we can use the most recent released version as a branch protection, and use the main branch one as an informative check that does not block PRs. The main one could even run on a nightly schedule and create an issue if there is a failure. I'm open to this option as a "stable" alternative that would give us much of the same benefit, but I'm okay with either option.

Thanks @codysoyland for writing this up - I'm ok with this alternative. It might be good to have the issue it creates auto-assigned to @sigstore/security-response-team so we get eyes on it via a private disclosure.

@codysoyland
Copy link
Member Author

Closing in favor of #3979

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.

2 participants