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

[ozym]

This is to fix a breaking change in the v4 artifact upload action. It requires unique names for uploads, but if the action is called multiple times in a run it will hit this error. Options are have a unique name per run, or simply not do the upload. I expect a future action that simply does test coverage will would be a simpler option if needed.

[wilsonjord]

Hey @ozym , I think I already addressed this issue with: 12c9a58 (the overwrite flag)

[jajera]

i think this is good to be optional. i'm not sure what is currently using this functionality and should we need to take action?

[wilsonjord]

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

[ozym]

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

[ozym]

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.

[Mossman1215]

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

[jajera]

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.

[wilsonjord]

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.

[ozym]

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.

[wilsonjord]

@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.

[ozym]

@wilsonjord not sure the overwrite flag is working in that case.

[wilsonjord]

i think this is good to be optional. i'm not sure what is currently using this functionality and should we need to take action?

@jajera I believe @CallumNZ was doing work on copying this artifact to s3 automatically, but I can't remember the status of this work

[ozym]

This is the error that's trying to be fixed.

Multiple search paths detected. Calculating the least common ancestor of all paths
The least common ancestor is /tmp. This will be the root directory of the artifact
With the provided path, there will be 2 files uploaded
Artifact name is valid!
Root directory input is valid!
Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run

[wilsonjord]

@wilsonjord not sure the overwrite flag is working in that case.

From the migration doc (https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md):

In v4, Artifacts are immutable unless deleted. To achieve this same behavior, you can use overwrite: true to delete the Artifact before a new one is created:

I'm almost certain that I tested this behavior as well; do you mind linking the workflow that produced the error?

[ozym]

@wilsonjord not sure the overwrite flag is working in that case.

From the migration doc (https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md):

In v4, Artifacts are immutable unless deleted. To achieve this same behavior, you can use overwrite: true to delete the Artifact before a new one is created:

I'm almost certain that I tested this behavior as well; do you mind linking the workflow that produced the error?

https://github.com/GeoNet/field/actions/runs/11563929821/job/32188174573

[ozym]

The problem isn't that it has been overwritten, it's because there are now two outputs with the same name being attempted (two different parts of the run, i.e. each with it's overwritten output).

[wilsonjord]

The problem isn't that it has been overwritten, it's because there are now two outputs with the same name being attempted (two different parts of the run, i.e. each with it's overwritten output).

I've been reading here: actions/upload-artifact#506
It may be what you are hitting. I didn't not test (or think) of this particular usecase.

[wilsonjord]

@ozym To keep behavior as close as possible to previous v3 behavior, I think simply adding continue-on-error would be justified. You end up with undetermined knowledge of what job actually produced the final artifact, but that is what we had previously with v3. The downside is that you may miss another class of errors, but I would suggest the Upload test log step is not critical enough to make that distinction.

If continue-on-error is not preferred, then your PR as is (that is, making test coverage upload opt-in), is justified considering that v4 no longer supports parallel uploads to the same artifact - once the parallel job usecase is solved, I'm sure they can be turned back on (fyi @CallumNZ )