Add support for downloading packs from GHES by aeisenberg · Pull Request #1221 · github/codeql-action · GitHub

This change adds:

new registries input in init action. This input is similar to the registries block in the qlconfig.yml file.
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:

integration test
handle pack downloads when the config is generated by the CLI

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

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

Close and re-open to kick the PR checks into starting.

OK...this should work now. The integration test is not ideal since it won't fail if both the registries-auth-tokens input and the registries block are ignored. In this case it will just fall back to using the token input.

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.

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.

@jf205 thoughts?

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.

Mostly minor comments — this generally looks in good shape!

Suggested change

      
                  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):

Suggested change

      
                  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.

I'm surprised to read this — shouldn't we ensure that the token input is only ever passed to the current GitHub instance?

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.

?

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 👍

Minor

Suggested change

      
                    token: "still-a-token",
          
                    token: "still-not-a-token",

Minor: Could add a // registries comment here so it's easier to see which input is undefined.

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 and RegistryConfigWithCredentials for instance.

Suggested change

      
                  `Invalid registries input. Must be a JSON string, but got: ${
          
                  `Invalid registries input. Must be a YAML string, but got: ${

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.

I'll remove the original error message from new new error.

Can we enforce the absence of token using the type system?

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.

…k-download

Looks reasonable, and I think you've addressed @henrymercer's comments. Some minor suggestions, and one reminder to add a CodeQL version number gate.

Shall we mention the registries field here briefly?

I see this entry is in the wrong version. I'll change that, too.

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.

Looks good! Assuming @jf205 is happy with the user-facing behaviour, this is good to go.

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.

@adityasharad would you mind taking another look. I added some better error handling and logic that ensures the registry urls end in/ to avoid the bug in 2.10.4.

Code changes looks good; linter checks are grumbling.

Avoids a bug in 2.10.4. Also, add some better handling for invalid registries blocks.

aeisenberg requested a review from a team as a code owner August 29, 2022 19:58

aeisenberg marked this pull request as draft August 29, 2022 20:16

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch 3 times, most recently from df07a38 to 8eba848 Compare August 29, 2022 20:48

Base automatically changed from aeisenberg/move-pack-download-to-init to main August 30, 2022 16:58

aeisenberg added 2 commits August 30, 2022 10:00

Update changelog

c7bb894

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch 3 times, most recently from 0308a5d to 01c45bf Compare August 30, 2022 18:03

aeisenberg closed this Aug 30, 2022

aeisenberg reopened this Aug 30, 2022

aeisenberg marked this pull request as ready for review August 30, 2022 19:06

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch from 01c45bf to db627c9 Compare August 30, 2022 19:39

aeisenberg marked this pull request as draft August 30, 2022 20:31

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch from 976e4e2 to 7fd7f72 Compare August 30, 2022 21:20

Add integration test for using registries-auth-tokens

1d92118

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch from 7fd7f72 to 1d92118 Compare August 30, 2022 22:56

aeisenberg marked this pull request as ready for review August 30, 2022 23:04

Change to using a single input

e9b47b1

henrymercer reviewed Sep 2, 2022

View reviewed changes

aeisenberg added 2 commits September 2, 2022 11:32

Merge branch 'main' into aeisenberg/ghes-pack-download

b044362

Convert from json to yaml for registries input

abdf26c

henrymercer reviewed Sep 6, 2022

View reviewed changes

aeisenberg added 2 commits September 6, 2022 10:22

Merge remote-tracking branch 'upstream/main' into aeisenberg/ghes-pac…

7e086b2

…k-download

Apply suggestions from code review

bf97a6d

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch from 1efca41 to b0e69f9 Compare September 6, 2022 17:46

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch 2 times, most recently from 0f06c56 to 0573cda Compare September 6, 2022 20:59

Clarify description of registries input

376fea6

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch from 0573cda to 376fea6 Compare September 6, 2022 21:06

adityasharad reviewed Sep 7, 2022

View reviewed changes

Comment thread init/action.yml

adityasharad reviewed Sep 7, 2022

View reviewed changes

aeisenberg added 2 commits September 7, 2022 14:38

Gate the new registries input behind version constraints

4fa3e8b

Update changelog and input descriptions

5dcb9de

adityasharad approved these changes Sep 7, 2022

View reviewed changes

aeisenberg added 2 commits September 7, 2022 15:15

Only run the init-with-registries check on nightly

aa434aa

Fix unit tests

5974446

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch from 88a78f0 to 5974446 Compare September 7, 2022 23:10

Append / to end of registries url

6085805

Avoids a bug in 2.10.4. Also, add some better handling for invalid registries blocks.

aeisenberg force-pushed the aeisenberg/ghes-pack-download branch from b1d4b13 to 6085805 Compare September 8, 2022 15:00

aeisenberg merged commit 9ba4d50 into main Sep 8, 2022

aeisenberg deleted the aeisenberg/ghes-pack-download branch September 8, 2022 17:02

This was referenced Sep 14, 2022

Merge main into releases/v2 #1246

Merged

Merge releases/v2 into releases/v1 #1248

Merged

+                    registries: |
+                      [
+                        {
+                          "url": "https://ghcr.io/v2/",
+                          "packages": "*/*",
+                          "token": "${{ secrets.GITHUB_TOKEN }}"
+                        }
+                      ]

+                  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):

+                      go: ["c", "d"],
+                      python: ["e", "f"],
+                    },
+                    undefined,

+                  return registriesInput ? yaml.l(registriesInput) : undefined;
+                } catch (e) {
+                  throw new Error(
+                    `Invalid registries input. Must be a JSON string, but got: ${

+                } catch (e) {
+                  throw new Error(
+                    `Invalid registries input. Must be a JSON string, but got: ${
+                      e instanceof Error ? e.message : String(e)

	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):

+                          packages: */*
+                          token: ${{ secrets.GHCR_TOKEN }}
+                    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.
	The url property contains the URL to the container registry you want to connect to.


		The token property contains a connection token for this registry.

		If this input is missing, the `token` input is used for all pack downloads.

+                    {
+                      url: "https://containers.GHEHOSTNAME1/v2/",
+                      packages: "semmle/*",
+                      token: "still-a-token",


		export type QueryFilter = ExcludeQueryFilter \| IncludeQueryFilter;

		export type RegistryConfig = SafeRegistryConfig & {

	`Invalid registries input. Must be a JSON string, but got: ${
	`Invalid registries input. Must be a YAML string, but got: ${

+              function createRegistriesBlock(registries: RegistryConfig[]) {
+                // be sure to remove the `token` field from the registry before writing it to disk.
+                const safeRegistries = registries.map((registry) => ({

               ## 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)

+                    !(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}.`

Add support for downloading packs from GHES by aeisenberg · Pull Request #1221 · github/codeql-action · GitHub

Add support for downloading packs from GHES - #1221

Add support for downloading packs from GHES#1221aeisenberg merged 14 commits intomaingithub/codeql-action:mainfrom aeisenberg/ghes-pack-downloadgithub/codeql-action:aeisenberg/ghes-pack-downloadCopy head branch name to clipboard

Conversation

aeisenberg commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge / deployment checklist

Uh oh!

aeisenberg commented Aug 30, 2022

Uh oh!

aeisenberg commented Aug 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aeisenberg commented Sep 7, 2022

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Sep 8, 2022

Uh oh!

adityasharad commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Add support for downloading packs from GHES#1221
aeisenberg merged 14 commits into
mainfrom
aeisenberg/ghes-pack-download

aeisenberg commented Aug 29, 2022 •
edited
Loading