-
Notifications
You must be signed in to change notification settings - Fork 385
Remove augmentationProperties
from Config
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fe428a8
to
e9fb72d
Compare
1ef359a
to
87c5b58
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.
Pull Request Overview
This PR removes the augmentationProperties
field from the Config
type by updating sendCompletedStatusReport
to use computed packs from computedConfig
instead of calculating them from UserConfig
and AugmentationProperties
. The change simplifies the configuration interface while maintaining the same functionality.
Key changes:
- Removed
augmentationProperties
from theConfig
type definition - Updated
getDefaultConfig
return type to temporarily includeaugmentationProperties
for internal use - Simplified pack calculation logic in
sendCompletedStatusReport
to usecomputedConfig.packs
directly
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 | Removes augmentationProperties from Config type and updates getDefaultConfig return type |
src/init-action.ts | Simplifies pack calculation logic to use config.computedConfig.packs directly |
src/testing-utils.ts | Removes augmentationProperties import and removes the field from test config creation |
src/config-utils.test.ts | Updates test to destructure augmentationProperties from getDefaultConfig result |
src/codeql.test.ts | Updates test to pass augmentationProperties directly to generateCodeScanningConfig |
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 the packs
property is populated correctly?
src/config-utils.ts
Outdated
@@ -1104,14 +1100,14 @@ export async function initConfig(inputs: InitConfigInputs): Promise<Config> { | |||
); | |||
} | |||
|
|||
const config = await getDefaultConfig(inputs); | |||
const { augmentationProperties, ...config } = await getDefaultConfig(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 use getDefaultConfig
in this function, consider having getDefaultConfig
populate computedConfig
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 the UserConfig
(from the inputs) an argument to getDefaultConfig
(now initActionState
), set it there, and generate computedConfig
there. That avoids having to return augmentationProperties
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!
This PR updates
sendCompletedStatusReport
to use the computed packs fromcomputedConfig
rather than computing the packs from the inputUserConfig
andAugmentationProperties
locally.As a result,
AugmentationProperties
is no longer needed inConfig
and is removed.Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist