Skip to content

Conversation

@sampart
Copy link
Contributor

@sampart sampart commented Jun 26, 2020

You can see an example of this in use in a branch that builds off this one. Here's a deep link to the Actions run, showing the HTTP request: https://github.com/github/codeql-action/runs/811641896?check_suite_focus=true#step:4:27

@sampart sampart requested a review from robertbrignull June 26, 2020 15:24
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

The code LGTM and the test that you included looks good too.

Only thing I can think of to improve it would be to add an entry to https://github.com/github/codeql-action/blob/main/.github/workflows/integration-testing.yml so it gets tested automatically as well.

@sampart
Copy link
Contributor Author

sampart commented Jun 30, 2020

Thanks @robertbrignull. I've had a go at adding an integration test in 4b37db7; I'd appreciate your eyes on that one, thanks! In particular, can we simplify the checkout?

@robertbrignull
Copy link
Contributor

I'd copy what the existing integration tests do, instead of using the format from the public template. There's no need to do the fetch-depth and custom checkout step for the integration tests. I'd just copy multi-language-repo_test-custom-queries but change the reference to the config file to be a remote repository reference.

@robertbrignull
Copy link
Contributor

Or you could even change that existing test so it tests both aspects at once. We don't have a good guideline for this. I'm wary of introducing too many extra tests, especially if they all test on multiple OS's, because the tests will start taking up too many action minutes and take too long. It's unclear if we've reached that point or not yet. And since we can always change the test setup at any point, whatever you do we can always adjust in the future.

@sampart
Copy link
Contributor Author

sampart commented Jun 30, 2020

Sounds like a plan, thanks. Done in 1bb294a.

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Thanks for adding the integration test. LGTM

@sampart sampart merged commit e2a8f32 into main Jun 30, 2020
@sampart sampart deleted the support-remote-config branch June 30, 2020 14:19
@github-actions github-actions bot mentioned this pull request Jul 6, 2020
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