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(repo): git clone output to Stderr #7561

Merged
merged 1 commit into from
Oct 8, 2024
Merged

fix(repo): git clone output to Stderr #7561

merged 1 commit into from
Oct 8, 2024

Conversation

dangogh
Copy link
Contributor

@dangogh dangogh commented Sep 19, 2024 •
edited
Loading

Description

This ensures that git clone output is directed to os.Stderr instead of os.Stdout. Test added to demonstrate that stdout is empty after clone.

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@CLAassistant
Copy link

CLAassistant commented Sep 19, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@dangogh
Copy link
Contributor Author

dangogh commented Sep 19, 2024

NOTE: currently unable to run mage test:unit due to an error: ../../wasm/sdk.go:146:10: cannot use uintptr(size) (value of type uintptr) as int value in assignment. Probably something due to my macbook pro M3 installlation. I'll bring up in discussions tomorrow if unable to resolve that.

@dangogh dangogh marked this pull request as ready for review September 24, 2024 22:23
knqyf263
knqyf263 reviewed Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

It seems that there's no simple way to add a test at the moment. Please feel free to revert it so we can merge this fix soon.

pkg/fanal/artifact/repo/git_test.go Outdated
func wrapNewArtifact(target string, c cache.ArtifactCache, fs *walker.FS, opts artifact.Option) (func(), []byte, error) {
orig := os.Stdout
r, w, _ := os.Pipe()
os.Stdout = w
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overriding os.Stdout could cause a problem if this test is run in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed -- reverted the test

knqyf263
knqyf263 approved these changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Thanks

@dangogh
Copy link
Contributor Author

dangogh commented Sep 25, 2024

Need some help here. Integration tests failed with this:

* remote error: GET https://ghcr.io/v2/aquasecurity/trivy-test-images/manifests/sha256:460fdaba6a3c934072bd8a6e74be574085a573d2cc920abaf470db0c43c7fffe: TOOMANYREQUESTS: retry-after: 281.482µs, allowed: 44000/minute

and it's not clear to me if I can simply restart the test.

@knqyf263
Copy link
Collaborator

Yes, it's a known issue. We've raised an issue, but we haven't heard back yet.
https://github.com/orgs/community/discussions/139074

auto-merge was automatically disabled October 7, 2024 14:43

Head branch was pushed to by a user without write access

@knqyf263 knqyf263 added this pull request to the merge queue Oct 8, 2024
Merged via the queue into aquasecurity:main with commit fdf203c Oct 8, 2024
12 checks passed
@dangogh dangogh deleted the fix/7547 branch October 8, 2024 17:43
@aqua-bot aqua-bot mentioned this pull request Oct 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knqyf263 knqyf263 knqyf263 approved these changes

@DmitriyLewen DmitriyLewen Awaiting requested review from DmitriyLewen DmitriyLewen is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix(repo): git CLI output mangles stdout
3 participants
@dangogh @CLAassistant @knqyf263