-
Notifications
You must be signed in to change notification settings - Fork 335
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 support for downloading packs from GHES #1221
Conversation
df07a38
to
8eba848
Compare
This change adds: - new `registries` block allowed in code scanning config file - new `registries-auth-tokens` input in init action - Change the downloadPacks function so that it accepts new parameters: - registries block - api auth - Generate a qlconfig.yml file with the registries block if one is supplied. Use this file when downloading packs. - temporarily set the `GITHUB_TOKEN` and `CODEQL_REGISTRIES_AUTH` based on api auth TODO: 1. integration test 2. handle pack downloads when the config is generated by the CLI
0308a5d
to
01c45bf
Compare
Close and re-open to kick the PR checks into starting. |
01c45bf
to
db627c9
Compare
976e4e2
to
7fd7f72
Compare
7fd7f72
to
1d92118
Compare
OK...this should work now. The integration test is not ideal since it won't fail if both the |
registries: | | ||
[ | ||
{ | ||
"url": "https://ghcr.io/v2/", | ||
"packages": "*/*", | ||
"token": "${{ secrets.GITHUB_TOKEN }}" | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully not too much of a bikeshed, but why JSON rather than YAML here? YAML seems more consistent with other configuration files like the registries
property in qlconfig.yml
files and codeql-workspace.yml
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that if we use JSON here, it's more explicit that this is another thing and we can say "JSON encoded string". People know what that means. However, if we use yaml here, we have to say: "well...you need to add an extra |
since inputs only accept strings, but want to parse yaml".
I do agree, though, that yaml would be easier to read. I am quite ambivalent about this and maybe we should get more feedback on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jf205 thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i prefer YAML. If we have with a nice clear example in the docs then i think this will be fine for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments — this generally looks in good shape!
init/action.yml
Outdated
required: false | ||
registries: | ||
description: | | ||
A YAML string that defines the list of GitHub container registries to use for downloading packs. The string is in the following forma (the | is required on the first line): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A YAML string that defines the list of GitHub container registries to use for downloading packs. The string is in the following forma (the | is required on the first line): | |
A YAML string that defines the list of GitHub container registries to use for downloading packs. The string is in the following form (the | is required on the first line): |
init/action.yml
Outdated
packages: */* | ||
token: ${{ secrets.GHCR_TOKEN }} | ||
|
||
The url property contains the url to the container registry you want to connect to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The url property contains the url to the container registry you want to connect to. | |
The url property contains the URL to the container registry you want to connect to. |
init/action.yml
Outdated
|
||
The token property contains a connection token for this registry. | ||
|
||
If this input is missing, the `token` input is used for all pack downloads. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised to read this — shouldn't we ensure that the token
input is only ever passed to the current GitHub instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean here. If the registries
input is missing, then we only connect to the current GitHub instance and we always use the token
input to connect.
Would it be sufficient to say something like:
If you only need to download packages from this GitHub instance, use the
token
input instead.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. I originally interpreted "the token
input is used for all pack downloads" to mean that we would pass the token
input to the registry for each pack. Your suggestion sounds a lot clearer 👍
src/config-utils.test.ts
Outdated
{ | ||
url: "https://containers.GHEHOSTNAME1/v2/", | ||
packages: "semmle/*", | ||
token: "still-a-token", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor
token: "still-a-token", | |
token: "still-not-a-token", |
src/config-utils.test.ts
Outdated
go: ["c", "d"], | ||
python: ["e", "f"], | ||
}, | ||
undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Could add a // registries
comment here so it's easier to see which input is undefined.
src/config-utils.ts
Outdated
@@ -61,6 +61,23 @@ export interface UserConfig { | |||
|
|||
export type QueryFilter = ExcludeQueryFilter | IncludeQueryFilter; | |||
|
|||
export type RegistryConfig = SafeRegistryConfig & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the terminology of safe vs unsafe is helpful in general, but two comments here:
- I think it might be more important to name the unsafe one unsafe vs the safe one safe, so we think more carefully whenever we see
Unsafe
in the code. - It might be clearer (though more verbose) to use
RegistryConfigNoCredentials
andRegistryConfigWithCredentials
for instance.
src/config-utils.ts
Outdated
return registriesInput ? yaml.l(registriesInput) : undefined; | ||
} catch (e) { | ||
throw new Error( | ||
`Invalid registries input. Must be a JSON string, but got: ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Invalid registries input. Must be a JSON string, but got: ${ | |
`Invalid registries input. Must be a YAML string, but got: ${ |
src/config-utils.ts
Outdated
} catch (e) { | ||
throw new Error( | ||
`Invalid registries input. Must be a JSON string, but got: ${ | ||
e instanceof Error ? e.message : String(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message might contain credentials. Actions will mask out any secrets, but perhaps it'd be safe practice to avoid printing the input in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove the original error message from new new error.
|
||
function createRegistriesBlock(registries: RegistryConfig[]) { | ||
// be sure to remove the `token` field from the registry before writing it to disk. | ||
const safeRegistries = registries.map((registry) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we enforce the absence of token
using the type system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add an explicit return type to the function, but there is no way to ensure the token is removed from the object using the type system alone.
1efca41
to
b0e69f9
Compare
0f06c56
to
0573cda
Compare
0573cda
to
376fea6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable, and I think you've addressed @henrymercer's comments. Some minor suggestions, and one reminder to add a CodeQL version number gate.
CHANGELOG.md
Outdated
@@ -7,6 +7,7 @@ No user facing changes. | |||
## 2.1.22 - 01 Sep 2022 | |||
|
|||
- Downloading CodeQL packs has been moved to the `init` step. Previously, CodeQL packs were downloaded during the `analyze` step. [#1218](https://github.com/github/codeql-action/pull/1218) | |||
- Allow CodeQL packs to be downloaded from GitHub Enterprise Server instances. [#1221](https://github.com/github/codeql-action/pull/1221) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we mention the registries
field here briefly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this entry is in the wrong version. I'll change that, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to link to the documentation when it is available, but that won't be out for a while.
Thanks for the comments. I addressed them all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Assuming @jf205 is happy with the user-facing behaviour, this is good to go.
!(await codeQlVersionAbove(codeQL, CODEQL_VERSION_GHES_PACK_DOWNLOAD)) | ||
) { | ||
throw new Error( | ||
`'registries' input is not supported on CodeQL versions less than ${CODEQL_VERSION_GHES_PACK_DOWNLOAD}.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just occurred to me: we may want to gate against the target GHES version too (if we're on GHES). Can be a follow-up; not critical for this PR.
88a78f0
to
5974446
Compare
@adityasharad would you mind taking another look. I added some better error handling and logic that ensures the registry urls end in |
Code changes looks good; linter checks are grumbling. |
Avoids a bug in 2.10.4. Also, add some better handling for invalid registries blocks.
b1d4b13
to
6085805
Compare
This change adds:
registries
input in init action. This input is similar to theregistries
block in theqlconfig.yml
file.GITHUB_TOKEN
andCODEQL_REGISTRIES_AUTH
based on api authTODO:
Merge / deployment checklist