-
Notifications
You must be signed in to change notification settings - Fork 411
Add getOptionalEnvVar helper
#3233
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
Also add tests for it and `getRequiredEnvParam`
| */ | ||
| export function getOptionalEnvVar(paramName: string): string | undefined { | ||
| const value = process.env[paramName]; | ||
| if (value?.trim().length === 0) { |
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.
Minor: this trims but getRequiredEnvParam doesn't — should we be consistent?
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.
Let's be inconsistent for now. Mainly because this would be a stricter requirement for getOptionalEnvVar than what we currently have and getOptionalEnvVar is only used in one (new) place for now where we don't risk breaking anything.
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 introduces a new getOptionalEnvVar helper function to safely retrieve environment variables that may not be set or may be empty, returning undefined in those cases. This complements the existing getRequiredEnvParam function and is immediately applied to simplify the logic in writePostProcessedFiles.
Key changes:
- Added
getOptionalEnvVarhelper function inutil.tswith appropriate test coverage - Refactored
writePostProcessedFilesto use the new helper instead of inline null/empty checks
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/util.ts | Added getOptionalEnvVar helper function |
| src/util.test.ts | Added comprehensive test coverage for both getRequiredEnvParam and getOptionalEnvVar |
| src/upload-lib.ts | Applied getOptionalEnvVar to simplify environment variable retrieval logic |
| lib/upload-sarif-action.js | Generated JavaScript from TypeScript changes |
| lib/upload-lib.js | Generated JavaScript from TypeScript changes |
| lib/analyze-action.js | Generated JavaScript from TypeScript changes |
Comments suppressed due to low confidence (3)
lib/upload-sarif-action.js:1
- The function
getOptionalEnvVaris being accessed with bracket notation instead of being called with parentheses. This should begetOptionalEnvVar(\"CODEQL_ACTION_SARIF_DUMP_DIR\")to properly invoke the function.
"use strict";
lib/upload-lib.js:1
- The function
getOptionalEnvVaris being accessed with bracket notation instead of being called with parentheses. This should begetOptionalEnvVar(\"CODEQL_ACTION_SARIF_DUMP_DIR\")to properly invoke the function.
"use strict";
lib/analyze-action.js:1
- The function
getOptionalEnvVaris being accessed with bracket notation instead of being called with parentheses. This should begetOptionalEnvVar(\"CODEQL_ACTION_SARIF_DUMP_DIR\")to properly invoke the function.
"use strict";
78dbf90 to
1ecd563
Compare
We have a helper function for inputs which gets the corresponding value, if it is set and not empty. We don't have this for environment variables. This PR adds
getOptionalEnvVarto address that.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning).analysis-kinds: code-quality).upload-sarif).How did/will you validate this change?
.test.tsfiles).pr-checks).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?
Merge / deployment checklist