-
Notifications
You must be signed in to change notification settings - Fork 62
generate credentials metadata #1502
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
Conversation
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.
Pull Request Overview
This PR moves the generation of credentials metadata from the Dependabot job definition into the Action by introducing a helper in the Updater class, and wires it through types and tests.
- Introduce
generateCredentialsMetadatainsrc/updater.tsand populatedetails['credentials-metadata'] - Extend
JobDetailsandCredentialinsrc/api-client.tswith optional metadata fields - Update unit and integration tests to include and validate the new
credentials-metadataproperty
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/updater.ts | Add generateCredentialsMetadata and set details['credentials-metadata'] in constructor |
| src/api-client.ts | Append 'credentials-metadata' to JobDetails and extend Credential type |
| tests/updater.test.ts | New spec to verify credentials metadata is correctly generated |
| tests/updater-builder-integration.test.ts | Include empty credentials-metadata in builder integration fixture |
| tests/main.test.ts | Add credentials-metadata stub to main test job details |
Comments suppressed due to low confidence (3)
src/updater.ts:61
- Consider adding a brief JSDoc comment above
generateCredentialsMetadatato describe its purpose and behavior for future maintainers.
private generateCredentialsMetadata(): Credential[] {
src/updater.ts:75
- [nitpick] The variable name
objis generic; renaming it to something likemetadataorfilteredCredentialwould improve readability.
const obj = {
tests/updater.test.ts:170
- The current test covers
hostandreplaces-base. Adding a credential entry in tests with other fields likeurlorenv-keywould ensure the metadata picks up all allowed keys.
it('generates credentials metadata on the job definition', () => {
| export type Credential = { | ||
| type: string | ||
| host: string | ||
| host?: string |
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.
Would host be optional?
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.
Yep, some ecosystems use host, some use url, some use both!
rickreyhsig
left a comment
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.
lgtm
|
While testing I found that it inserted a lot of duplicate metadata from our test org which made the job definition quite long. To match the old ways I've de-duped the metadata. I also removed the jit_access type which is new and the Updaters don't use directly. I will now re-test. |
|
This looks good now, the metadata matches what the server sends. |
We're adding new ways to provide credentials to the Dependabot job. The job definition contains metadata for the credentials, in order to do things like generate an npmrc on-the-fly.
To simplify things, we're moving the metadata generation to the Action. It's the perfect place to do it, right before starting the job we have all the information we need to generate it.
For safety we use an "allow list" strategy, that way newly added credentials will not suddenly appear in the job definition.