-
Notifications
You must be signed in to change notification settings - Fork 825
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
feat: added new fields to usage data #6911
feat: added new fields to usage data #6911
Conversation
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 with some optional nits.
Added * project setting * accountId
db5cd97
to
221a8bd
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.
left a few nits and some other clarifying questions
packages/amplify-cli/src/domain/amplify-usageData/NoUsageData.ts
Outdated
Show resolved
Hide resolved
this.version = version; | ||
this.inputOptions = input.options ? _.pick(input.options as InputOptions, ['sandboxId']) : {}; |
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.
is this only storing the sandboxId? if so, I'd just name it that. Also, does this simplify to:
this.inputOptions = input.options ? _.pick(input.options as InputOptions, ['sandboxId']) : {}; | |
this.inputOptions = input?.options?.sandboxId || {}; |
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.
No we are going to extend this in the future, this is there to add more field if needed in the future
packages/amplify-cli/src/domain/amplify-usageData/UsageDataPayload.ts
Outdated
Show resolved
Hide resolved
packages/amplify-provider-awscloudformation/src/aws-utils/aws-sts.ts
Outdated
Show resolved
Hide resolved
👋 Hi, this pull request was referenced in the v4.48.0 release! Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.48.0. |
Issue #, if available:
Description of changes:
Added additional fields to the usage data payload.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.