Skip to content

CI: keep version coverage and checks reliable - #1941

Closed
mattjohnsonpint wants to merge 1 commit into
stretchr:masterfrom
mattjohnsonpint:mjp/ci
Closed

mattjohnsonpint wants to merge 1 commit into
stretchr:masterfrom
mattjohnsonpint:mjp/ci

Conversation

@mattjohnsonpint

Copy link
Copy Markdown
Contributor

As stable and oldstable advance to Go 1.27 and Go 1.26, retain explicit Go 1.25 coverage so every supported release remains tested. Run language-level formatting validation with the stable toolchain because Go 1.26 cannot parse generic methods, even in files excluded by build constraints.

Several CI helpers could previously report success when their underlying commands, generation checks, or action-pin validation failed. Ensure those failures are propagated and newly generated files are detected so a green build means the checks actually completed successfully.

As stable and oldstable advance to Go 1.27 and Go 1.26, retain explicit Go 1.25 coverage so every supported release remains tested. Run language-level formatting validation with the stable toolchain because Go 1.26 cannot parse generic methods, even in files excluded by build constraints.

Several CI helpers could previously report success when their underlying commands, generation checks, or action-pin validation failed. Ensure those failures are propagated and newly generated files are detected so a green build means the checks actually completed successfully.

@dolmen dolmen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Too many unrelated changes in a single commit.

Please split in multiple commits (or even multiple PRs as I don't think that changing the workflow is related to the changes in the scripts). Each commit must explain the intent of the change.

Comment thread .ci.gofmt.sh
Comment on lines +5 to +6
unformatted_files="$(gofmt -l .)"
if [ -n "$unformatted_files" ]; then

@dolmen dolmen Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change is an example of making the review harder than necessary:

  • this is not a functional change: dropping that change would have no impact, but the reviewer has to check that
  • as this is mixed with plenty of other changes, the reviewer has to check how those changes interact. If that single change was in a single commit that tells the intent, it would be clearer that the change is contained
  • the refactor is just to clarify the action done by this test. But just adding a comment before the code (instead of changing the code) would have been easier to review as there would be no question about interaction with other changes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll revise, simplify and split up as necessary for clarity.

@mattjohnsonpint
mattjohnsonpint marked this pull request as draft September 1, 2026 14:52
@mattjohnsonpint

Copy link
Copy Markdown
Contributor Author

Closing this PR. I'll create a few new ones. Thanks.

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