- 
                Notifications
    You must be signed in to change notification settings 
- Fork 412
          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 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 | Removes augmentationPropertiesfromConfigtype and updatesgetDefaultConfigreturn type | 
| src/init-action.ts | Simplifies pack calculation logic to use config.computedConfig.packsdirectly | 
| src/testing-utils.ts | Removes augmentationPropertiesimport and removes the field from test config creation | 
| src/config-utils.test.ts | Updates test to destructure augmentationPropertiesfromgetDefaultConfigresult | 
| src/codeql.test.ts | Updates test to pass augmentationPropertiesdirectly 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 the packs property is populated correctly?
        
          
                src/config-utils.ts
              
                Outdated
          
        
      | } | ||
|  | ||
| 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
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