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

chore: add logging to snyk code bundles for troubleshooting #700

Merged
merged 16 commits into from
Oct 17, 2024
Merged

chore: add logging to snyk code bundles for troubleshooting #700

merged 16 commits into from
Oct 17, 2024

Conversation

bastiandoetsch
Copy link
Collaborator

Description

Provide description of this PR and changes, if linked Jira ticket doesn't cover it in full.

Checklist

  • Tests added and all succeed
  • Linted
  • README.md updated, if user-facing
  • License file updated, if new 3rd-party dependency is introduced

@bastiandoetsch bastiandoetsch requested a review from a team as a code owner October 11, 2024 15:29
@bastiandoetsch bastiandoetsch changed the title chore: add trace logging to snyk code bundles chore: add logging to snyk code bundles for troubleshooting Oct 11, 2024
cmars
cmars reviewed Oct 11, 2024
View reviewed changes
Copy link
Contributor

@cmars cmars left a comment

Choose a reason for hiding this comment

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

The logging changes seem reasonable but there are other drive-by changes here I have some concerns about, see below.

infrastructure/code/snyk_code_http_client.go Outdated Show resolved Hide resolved
infrastructure/code/snyk_code_http_client.go Outdated Show resolved Hide resolved
@bastiandoetsch bastiandoetsch force-pushed the chore/add-snyk-code-trace-logging-for-bundles branch from 765ee31 to 6447550 Compare October 15, 2024 12:48
cmars
cmars approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@cmars cmars left a comment

Choose a reason for hiding this comment

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

Question about some test changes below. I'd still like to see those landed as separate commits (if not separate PRs) for better isolation & bisect-ability but I'm not going to block this any further.

application/di/test_init.go
@@ -53,6 +54,8 @@ func TestInit(t *testing.T) {
defer initMutex.Unlock()
t.Helper()
c := config.CurrentConfig()
// we want to isolate CLI fake installs
c.CliSettings().SetPath(filepath.Join(t.TempDir(), "fake-cli"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this (and/or the server test changes below) fix the test flakes I've seen around the CLI downloads? :excited-conan-obrien-emoji:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope so - had to add it at a few places.

@bastiandoetsch bastiandoetsch merged commit 68655f0 into main Oct 17, 2024
17 checks passed
@bastiandoetsch bastiandoetsch deleted the chore/add-snyk-code-trace-logging-for-bundles branch October 17, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmars cmars cmars approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@bastiandoetsch @cmars