Allow artifact path to be located outside the sync root #2128
Conversation
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
There was a problem hiding this comment.
Dummy file to keep the directory around.
This is needed now that path translation verifies that artifact paths exist.
| assert.Equal(t, "artifact_a", relA) | ||
| relB, err := filepath.Rel(b.SyncRootPath, b.Config.Artifacts["test_b"].Path) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, filepath.Join("subfolder", "artifact_b"), relB) |
There was a problem hiding this comment.
Artifact paths are now absolute.
There was a problem hiding this comment.
assert.Equal(t, filepath.Join("subfolder", "artifact_b"), relB)
Artifact paths are now absolute.
What are you referring to? the variables in the test are relative.
There was a problem hiding this comment.
Simplified in latest.
| } | ||
| } | ||
|
|
||
| return dyn.InvalidValue, err |
There was a problem hiding this comment.
Denormalize the previous rewriteRelativeTo function into the body for jobs and pipelines.
It is not relevant outside these two resources.
| } | ||
| } | ||
|
|
||
| return dyn.InvalidValue, err |
There was a problem hiding this comment.
Denormalize the previous rewriteRelativeTo function into the body for jobs and pipelines.
It is not relevant outside these two resources.
There was a problem hiding this comment.
Can this and below comments be incorporated into source? Seems like valuable context.
There was a problem hiding this comment.
It's included (vs the previous), here:
// If we failed to rewrite the path, try to rewrite it relative to the fallback directory. // We only do this for jobs and pipelines because of the comment in [gatherFallbackPaths].
From gatherFallbackPaths the rest of the context is clear.
I would like to deprecate it but we don't know how much usage is out there. Another good thing to track when we can capture some telemetry.
| } else { | ||
| t.remoteRoot = t.b.Config.Workspace.FilePath | ||
| } | ||
|
|
There was a problem hiding this comment.
This assignment now happens once instead of for every path.
pietern
requested review from
andrewnester,
shreyas-goenka and
denik
as code owners
You can also do deploy in acceptance tests. You would just need to implement a few more dummy handlers in server_test.go. Another option is to extract 'build' into separate command. |
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Manually confirmed with pending acceptance tests that this does not change behavior or output. |
Changes
We perform a check during path translation that the path being referenced is contained in the bundle's sync root. If it isn't, it's not a valid remote reference. However, this doesn't apply to paths that are always local, such as the artifact path. An artifact's build command is executed in its path. Files created by the artifact build (e.g. wheels or JARs) don't need to be in the sync root because they have a dedicated and different upload path into
${workspace.artifact_path}.Therefore, this check that a path is contained in the bundle's sync root doesn't apply to artifact paths. This change modifies the structure of path translation to allow opting out of this check.
Fixes #1927.
Tests