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.

Sign up for GitHub

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

Jump to bottom

fix: add a go-test option to upload coverage artifacts #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: add a go-test option to upload coverage artifacts #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .github/workflows/reusable-go-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ on:
type: string
description: |
extra args to pass `go test`
upload:
required: false
default: false
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: this is a breaking change. Current behavior is true. I doubt anyone's workflow would break, but I don't know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's breaking change to stop the breaking change that stopped what was working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is all about the test coverage output that goes at the bottom of the actions summary page, it isn't an artifact that we store it's more of an overview. But because we have actions that call this multiple times then it hits this bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

as a compromise point can @ozym make the default true and pass in false in the workflows that have impacted your work?

Copy link

Choose a reason for hiding this comment

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

as a compromise point can @ozym make the default true and pass in false in the workflows that have impacted your work?

oh yeah, i think this is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's breaking change to stop the breaking change that stopped what was working.

If this feature is needed to fixed what stopped working, sure. I can spend time figuring out what broke, how I could have caught it when doing the PR, and making the overwrite flag work the way we want it to work, at a later point.
FWIW: when creating unique names, I've used ${{ github.run_id }}${{ github.run_attempt }} as the disambiguation piece of information.

Copy link
Contributor Author

@ozym ozym Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The problem for setting it to true is that it will have a much wider impact, it can be done obviously. But would need to be added to all the actions that embed this action.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ozym setting it to false by default is ok. The "copy to s3" work needs to be revisited anyway due to the maximum reusable nesting limit of 4, so we can deal with the "optional" presence of the artifact in the rework.

type: boolean
description: |
set to true to push test coverage as an artifact

jobs:
go-test:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -41,7 +48,7 @@ jobs:
go tool cover -html=/tmp/coverage.out -o /tmp/coverage.html
- name: Upload test log
uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3
if: always()
if: ${{ inputs.upload }}
with:
name: test-results
path: |
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ jobs:
uses: GeoNet/Actions/.github/workflows/reusable-go-test.yml@main
```

test coverage results upload to job artifacts, found at the bottom of a job summary page.
test coverage results can be uploaded to job artifacts, found at the bottom of a job summary page.

### Go vulnerability check

Expand Down
Loading