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.

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

env_*: log command output #69

Merged
merged 1 commit into from
Apr 18, 2024
Merged

env_*: log command output #69

merged 1 commit into from
Apr 18, 2024

Conversation

squat
Copy link
Contributor

@squat squat commented Sep 6, 2023

There are two instances where we forget to check the output of a command
when it fails. This commit ensures that we log STDOUT/STDERR in the same
way that we do whenever we capture CombinedOutput. Specifically, this
commit adds logging for the output when we fail to recursively chmod
the shared directory as part of the cleanup process.
One note here: recursively chmod-ing the shared directory could easily
fail if a container saves a file as root and the CI user is not root. We
might want to forget about chmod-ing and instead do a rm -rf, which
will delete files not owned by the user executing the command.

Signed-off-by: Lucas Servén Marín lserven@gmail.com

There are two instances where we forget to check the output of a command
when it fails. This commit ensures that we log STDOUT/STDERR in the same
way that we do whenever we capture `CombinedOutput`. Specifically, this
commit adds logging for the output when we fail to recursively chmod
the shared directory as part of the cleanup process.
One note here: recursively chmod-ing the shared directory could easily
fail if a container saves a file as root and the CI user is not root. We
might want to forget about chmod-ing and instead do a `rm -rf`, which
will delete files not owned by the user executing the command.

Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
squat added a commit to wavyland/wavy that referenced this pull request Sep 7, 2023
This commit ensures that the cleanup of the e2e tests is clean. It
removes the capture.png file, which is not chmod-able to ensure the the
cleanup of the Kind cluster does not fail. This is a workaround until a
better solution, like the one mentioned in
efficientgo/e2e#69 is available.

Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
Copy link
Collaborator

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Generally LGTM!
But I'm not sure why the macOS tests fail. The failures seem unrelated.

@squat
Copy link
Contributor Author

squat commented Sep 8, 2023

Yeah! It looks like the Mac tests can't pull Quay images, or so being able to pull after some time, but only with Kind! This could be some kind of ordering issue, since the Kind tests on Mac are going to be the ones that start the latest.

Copy link
Contributor

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Not sure how this could impact test, so merging. Thanks!

@bwplotka bwplotka merged commit c5a4b7c into efficientgo:main Apr 18, 2024
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.

3 participants