Skip to content

Allow config files to be from a remote repository - #87

Merged
sampart merged 24 commits into
mainfrom
support-remote-config
Jun 30, 2020
Merged

Allow config files to be from a remote repository#87
sampart merged 24 commits into
mainfrom
support-remote-config

Conversation

@sampart

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/config-utils.ts

@robertbrignullrobertbrignull left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@robertbrignullrobertbrignull left a comment

Copy link
Copy Markdown
Contributor

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 mainJun 30, 2020
@sampart
sampart deleted the support-remote-config branch June 30, 2020 14:19
@github-actionsgithub-actionsBot mentioned this pull request Jul 6, 2020
Sign up for freeto 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.

2 participants

@sampart@robertbrignull