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

Add JF_MSI env var #883

Merged
merged 49 commits into from
Jan 22, 2024
Merged

Conversation

orz25
Copy link
Contributor

@orz25 orz25 commented Dec 27, 2023

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • All static analysis checks passed.
  • This pull request is on the dev branch.
  • I used gofmt for formatting the code before submitting the pull request.

Analyzer Manager is expecting to get the multi_scan_id as env variable named JF_MSI. Added this variable right after we get the multi_scan_id from xsc.

PR in jfrog-cli-security using this branch as jfrog-client-go package (for test purposes):
jfrog/jfrog-cli-security#5

@eyalbe4 eyalbe4 self-requested a review December 28, 2023 23:08
@eyalbe4 eyalbe4 added the safe to test Approve running integration tests on a pull request label Dec 28, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Dec 28, 2023
Copy link
Contributor

👍 Frogbot scanned this pull request and found that it did not add vulnerable dependencies.


Copy link
Contributor

@eyalbe4 eyalbe4 left a comment

Choose a reason for hiding this comment

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

Before we can merge this PR, you should go ahead and create two additional PRs for the jfrog-cli-core and jfrog-cli projects, so that we can see that the tests that those projects include, and may be impacted by this change, as passing.
You'll need to use the replace statements in the go.mod files of those projects, to reference this PR's branch (your fork) for both of your jfrog-client-go and jfrog-cli-core. It'll be helpfull if you link all of the 3 PRs by adding referencing them in the descriptions of the PRs.

Comment on lines 115 to 118
err = os.Setenv("JF_MSI", multiScanId)
if err != nil {
return "", fmt.Errorf("failed setting msi as environment variable")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The above code can be simplified as follows. In addition, the error created should be wrapped with errorutils.CheckErrorf(err) iin this case, beucase os.Setenv, which may return the error, is an external API to the jfrog-client-go codebase. Also notice that in the above code the original error is lost, and the below code fixes that as well.

if err = os.Setenv("JF_MSI", multiScanId); err != nil {
	errorutils.CheckErrorf("failed setting msi as environment variable. Cause: %s", err.Error())
}

@eyalbe4 eyalbe4 self-requested a review December 29, 2023 09:44
@eyalbe4 eyalbe4 added the safe to test Approve running integration tests on a pull request label Jan 4, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 4, 2024
xray/services/scan.go Outdated Show resolved Hide resolved
@eyalbe4 eyalbe4 self-requested a review January 4, 2024 15:14
xray/services/scan.go Outdated Show resolved Hide resolved
xray/services/scan.go Outdated Show resolved Hide resolved
@attiasas attiasas added the safe to test Approve running integration tests on a pull request label Jan 22, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 22, 2024
@attiasas attiasas merged commit cd958f6 into jfrog:dev Jan 22, 2024
17 of 19 checks passed
@attiasas attiasas added the bug Something isn't working label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.