Add tests for shell execution on artifacts build #2884
Conversation
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| // Variant of [Execv] that runs the given script through a shell | ||
| func ShellExecv(content, dir string, env []string) error { | ||
| newOpts, err := shellExecvOpts(content, dir, env) | ||
| opts, err := shellExecvOpts(content, dir, env) |
There was a problem hiding this comment.
small typo fix based on: #2862 (comment)
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
|
||
| # Disable on Mac | ||
| #mac = false | ||
| #darwin = false |
There was a problem hiding this comment.
Fixes small typo, Mac OS is "darwin", not "mac" for GOOS. You need to set darwin = false for this to work.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
changed the title
shreyas-goenka
requested review from
andrewnester,
anton-107,
denik and
pietern
as code owners
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
How escape characters are interpreted by default depends on the shell.
But why are you testing that behavior here? It adds nuance to these test cases that isn't instrumental for the logic we implement. If you drop the echo with the newlines from the test, all comments about newlines can be removed, and we retain the same level of coverage from my pov.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
@pietern That was intentionally added to assert that the shell a user chooses in I looked into ways to do this assertion and the new line method was the best I could find. |
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
|
||
| >>> [CLI] bundle deploy | ||
| Building my_artifact... | ||
| Warn: my_artifact: Build succeeded |
There was a problem hiding this comment.
FYI, this message was removed in #2987 so you probably need to regenerate the tests.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
## Changes This PR inlines the script content using the `-c` flag in bash and sh instead of passing them in via `-e <file>`. ## Why Based on discussion in #2862 (comment). Getting rid of the temp file means we no longer have to clean up the temporary file created. ## Tests Tests that were added in: #2884
Why
This part of the codebase was missing test coverage. I plan on changing the execution of commands to inline the script content and these tests are required to ensure I don't break anything (relevant discussion: #2862 (comment))