| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Warning
This PR extends remote configuration file support by enhancing getRemoteConfig to accept shorthand remote addresses (optional owner, path, and ref with sensible defaults), and refactors environment access to be more testable via an Env wrapper.
Changes:
| File | Description |
|---|---|
| src/util.ts | Adds Env accessor helpers and refactors env var getters. |
| src/testing-utils.ts | Adds getTestEnv() and tightens typing for default Actions env vars. |
| src/environment.ts | Introduces Env interface abstraction for environment access. |
| src/config/remote-file.ts | New parser for shorthand remote config references and defaults. |
| src/config/remote-file.test.ts | Unit tests for remote file address parsing and defaults. |
| src/config/file.ts | Adds getRemoteConfig() using parsed remote address components. |
| src/config-utils.ts | Switches to the new getRemoteConfig() implementation. |
| src/config-utils.test.ts | Removes a test that no longer matches the supported shorthand syntax. |
| src/api-client.ts | Replaces raw env var names with ActionsEnvVars. |
| src/actions-util.ts | Adds ActionsEnvVars enum and updates env var usages. |
| lib/entry-points.js | Changed but excluded from review (generated/contents unavailable). |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks solid! I left a few comments, but minor stuff.
Sorry, something went wrong.
| ref: DEFAULT_CONFIG_FILE_REF, | ||
| } satisfies RemoteFileAddress); | ||
|
|
||
| t.deepEqual(parseRemoteFileAddress(env, "owner/repo/path@"), { |
There was a problem hiding this comment.
I think this should be invalid, for the same reason that /repo@ref is invalid.
Edit: Also, I just noticed the expected format below does not seem to indicate that this should be a correct format:
Expected format [<owner>/]<repository>[/<file-path>][@<ref>]
Sorry, something went wrong.
There was a problem hiding this comment.
I have no strong feelings on this, but agree that we should be consistent. I can change this to also be invalid.
Sorry, something went wrong.
There was a problem hiding this comment.
I left a few comments, most of them minor/simple.
If you're wondering about the Outdated label on the comments... I commented on the individual commits, because I reviewed them chronologically.
Sorry, something went wrong.
| { input: "owner/repo @ref:path", expected }, | ||
| { input: "owner/repo@ ref:path", expected }, | ||
| { input: "owner/repo@ref :path", expected }, | ||
| { input: "owner/repo@ref: path", expected }, |
There was a problem hiding this comment.
Whitespace is significant in a file path. How do you know that path is a typo, and that the user—for whatever reason—didn't actually intend path?
But, taking a step back, I don't think we should necessarily support (or expect) whitespace in the address. I think it's perfectly acceptable to treat a typo/mistake as an error, if it is, in fact, a mistake. That would allow us to treat owner , repo, ref literally and simplify the logic a bit.
Sorry, something went wrong.
There was a problem hiding this comment.
Whitespace is significant in a file path.
The path is used in getRemoteConfig as argument to the path parameter for the rest.repos.getContent call. I am not sure that leading or trailing whitespace would be meaningful there, or how happy git/GitHub is with repos containing whitespace at the start / end of file paths. If that is supported, then it would of course make sense to keep it.
Sorry, something went wrong.
There was a problem hiding this comment.
I am not sure that leading or trailing whitespace would be meaningful there, or how happy git/GitHub is with repos containing whitespace at the start / end of file paths.
I would expect git/GitHub to do its own input validation. And error appropriately if whitespace is invalid.
Sorry, something went wrong.
| const pieces = OLD_REMOTE_ADDRESS_FORMAT.exec(input); | ||
|
|
||
| // 5 = 4 groups + the whole expression | ||
| if (pieces?.groups === undefined || pieces.length < 5) { |
There was a problem hiding this comment.
pieces.length < 5 is a useless condition, because it will always be false. The regular expression pattern will either match with 4 groups + the whole expression, or it will not match at all.
In fact, pieces.length will also never be greater than 5.
Sorry, something went wrong.
There was a problem hiding this comment.
This is a check kept from the previous implementation and not new in this PR.
Sorry, something went wrong.
| * All the components are required. Unchanged from the previous implementation. | ||
| */ | ||
| const OLD_REMOTE_ADDRESS_FORMAT = new RegExp( | ||
| "(?<owner>[^/]+)/(?<repo>[^/]+)/(?<path>[^@]+)@(?<ref>.*)", |
There was a problem hiding this comment.
Sorry, something went wrong.
There was a problem hiding this comment.
What you wrote mirrors what Copilot complained about originally when I updated the regex to make parts of it optional. However, now that I have added a new regex for the new format, OLD_REMOTE_ADDRESS_FORMAT is just the regex from the existing code before any changes in this PR. In other words, making these adjustments (while perhaps reasonable) would alter the original behaviour. The old behaviour is not FF-gated, and so we'd be making a breaking change to existing code. That seems unnecessary to me.
Sorry, something went wrong.
There was a problem hiding this comment.
Warning
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This is a follow-up to #3963 which modifies getRemoteConfig to accept shorthand addresses for remote files. Concretely, this adds a new format (gated behind a FF) where the owner, path, and ref components are optional and default to the owner of the current repo (as given by GITHUB_REPOSITORY), .github/codeql-action.yaml, and main respectively.
Examples:
Notes for reviewers
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist