-
Notifications
You must be signed in to change notification settings - Fork 33
chore: remove audit user since it cannot be shown in ui #891
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
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 auditUserId feature from the deploy command, as this user identifier cannot be displayed in the UI. The change eliminates unused functionality related to extracting and passing user IDs from JWT tokens during deployment operations.
Key changes:
- Removed
auditUserIdextraction and passing from the deploy workflow - Removed
getTokenDataimport from deploy command (function is no longer called) - Cleaned up related test cases that validated
auditUserIdbehavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/commands/app/deploy.js | Removed getTokenData import and eliminated code that extracted user_id from JWT tokens and passed it as auditUserId to deployment configuration |
| test/commands/app/deploy.test.js | Removed five test cases validating auditUserId behavior with different token states (valid user_id, undefined, null, empty string, and null token data) and cleaned up associated mock setup |
| test/commands/lib/auth-helper.test.js | Removed test case for successful JWT token decoding via getTokenData |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sandeep-paliwal
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. Some comments from Co-pilot to be resolved before merging.
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Removing this value that we were propagating all the way to S3, the UI cannot resolve the user so this information is not required. Logs cover who deployed and when, so the data is there.
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: