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

Go / configure-baseline: account for multiple vendor directories and the CODEQL_EXTRACTOR_GO_EXTRACT_VENDOR_DIRS setting #17227

Merged

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Aug 14, 2024

Our existing configure-baseline scripts would give the wrong result if a vendor directory wasn't at the root of the repository, or if the CODEQL_EXTRACTOR_GO_EXTRACT_VENDOR_DIRS variable was set to true indicating the user wants their vendored code scanned.

Here I replace the shell scripts that implemented the very simplest behaviour with a small Go program.

@smowton smowton requested a review from a team as a code owner August 14, 2024 17:00
@github-actions github-actions bot added the Go label Aug 14, 2024
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

It generally looks good. I suppose that ideally we'd share the logic between the autobuilder and this script somehow, so that if we change one in future then the other changes automatically, but that probably introduces more complexity than we need.

go/extractor/configurebaseline/configurebaseline.go Outdated Show resolved Hide resolved
go/extractor/configurebaseline/configurebaseline.go Outdated Show resolved Hide resolved
go/codeql-tools/configure-baseline.cmd Outdated Show resolved Hide resolved
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

This generally looks reasonable. I have a few minor suggestions for improvements in what's already here. As you said, this does need tests. It would be good to have some unit tests for the logic in configurebaseline.go and then a couple of integration tests. We should also run QA later.

A bigger question I have is whether we can reasonably assume that the go-configure-baseline binary will always be available when the configure-baseline.sh script is called. I assume that's probably the case, but it may be worth checking if you haven't yet.

go/extractor/configurebaseline/configurebaseline.go Outdated Show resolved Hide resolved
go/extractor/configurebaseline/configurebaseline.go Outdated Show resolved Hide resolved
go/extractor/configurebaseline/configurebaseline.go Outdated Show resolved Hide resolved
go/extractor/configurebaseline/configurebaseline.go Outdated Show resolved Hide resolved
…the `CODEQL_EXTRACTOR_GO_EXTRACT_VENDOR_DIRS` setting

Our existing configure-baseline scripts would give the wrong result if a `vendor` directory wasn't at the root of the repository, or if the `CODEQL_EXTRACTOR_GO_EXTRACT_VENDOR_DIRS` variable was set to `true` indicating the user wants their vendored code scanned.

Here I replace the shell scripts that implemented the very simplest behaviour with a small Go program.
@smowton smowton force-pushed the smowton/fix/baseline-vs-nonroot-vendor-dirs branch from a8d6e64 to 3acab64 Compare August 20, 2024 16:07
@smowton
Copy link
Contributor Author

smowton commented Aug 20, 2024

@owen-mc this is now ready for re-review; all comments have been addressed and an integration test added.

I will run QA just in case it highlights anything breaking, but I'm aware this won't particularly exercise the code coverage information, only detect crashes.

@owen-mc
Copy link
Contributor

owen-mc commented Aug 20, 2024

The windows integration test seems to be failing on two counts: wrong separator and actually getting the wrong results.

@owen-mc
Copy link
Contributor

owen-mc commented Aug 21, 2024

(Don't forget to run QA, if you still plan to do that.)

@smowton
Copy link
Contributor Author

smowton commented Aug 21, 2024

@smowton
Copy link
Contributor Author

smowton commented Aug 22, 2024

QA is unremarkable (though it has a limited scope to show anything relevant to this change, it at least confirms there are no new surprising extraction failures)

@smowton smowton merged commit 67d9437 into github:main Aug 22, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants