- Notifications
You must be signed in to change notification settings - Fork413
RemoveaugmentationProperties fromConfig type#3075
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fe428a8 toe9fb72dCompare1ef359a to87c5b58CompareThere 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 removes theaugmentationProperties field from theConfig type by updatingsendCompletedStatusReport to use computed packs fromcomputedConfig instead of calculating them fromUserConfig andAugmentationProperties. The change simplifies the configuration interface while maintaining the same functionality.
Key changes:
- Removed
augmentationPropertiesfrom theConfigtype definition - Updated
getDefaultConfigreturn type to temporarily includeaugmentationPropertiesfor internal use - Simplified pack calculation logic in
sendCompletedStatusReportto usecomputedConfig.packsdirectly
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/config-utils.ts | RemovesaugmentationProperties fromConfig type and updatesgetDefaultConfig return type |
| src/init-action.ts | Simplifies pack calculation logic to useconfig.computedConfig.packs directly |
| src/testing-utils.ts | RemovesaugmentationProperties import and removes the field from test config creation |
| src/config-utils.test.ts | Updates test to destructureaugmentationProperties fromgetDefaultConfig result |
| src/codeql.test.ts | Updates test to passaugmentationProperties directly togenerateCodeScanningConfig |
| lib/init-action.js | Generated JavaScript file (not reviewed per guidelines) |
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.
Before merging, could you run some of the PR checks that include packs in debug mode and check that thepacks property is populated correctly?
src/config-utils.ts Outdated
| } | ||
| constconfig=awaitgetDefaultConfig(inputs); | ||
| const{ augmentationProperties, ...config}=awaitgetDefaultConfig(inputs); |
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.
Given that we only usegetDefaultConfig in this function, consider havinggetDefaultConfig populatecomputedConfig directly to simplify the flow.
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 have made theUserConfig (from the inputs) an argument togetDefaultConfig (nowinitActionState), set it there, and generatecomputedConfig there. That avoids having to returnaugmentationProperties as well.
@henrymercer I refactored creating the |
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.
Great, even better!
dfb741d intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR updates
sendCompletedStatusReportto use the computed packs fromcomputedConfigrather than computing the packs from the inputUserConfigandAugmentationPropertieslocally.As a result,
AugmentationPropertiesis no longer needed inConfigand is removed.Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist