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
chore: ask for consent for diagnose report #12710
base: dev
Are you sure you want to change the base?
Conversation
packages/amplify-e2e-tests/src/__tests__/migration/api.connection.migration.test.ts
Fixed
Show fixed
Hide fixed
packages/amplify-e2e-tests/src/__tests__/migration/api.connection.migration2.test.ts
Fixed
Show fixed
Hide fixed
packages/amplify-e2e-tests/src/__tests__/migration/api.key.migration1.test.ts
Fixed
Show fixed
Hide fixed
packages/amplify-e2e-tests/src/__tests__/migration/api.key.migration3.test.ts
Fixed
Show fixed
Hide fixed
packages/amplify-e2e-tests/src/__tests__/migration/api.key.migration3.test.ts
Fixed
Show fixed
Hide fixed
packages/amplify-e2e-tests/src/__tests__/migration/api.key.migration5.test.ts
Fixed
Show fixed
Hide fixed
false, | ||
false, | ||
0, | ||
false, | ||
true, |
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.
We should make a settings object within amplifyPushUpdate parameters.. I wrote this like this as a quick and dirty solution just to see if this would work.
if (DebugConfig.Instance.promptSendReport()) { | ||
const result = await prompter.yesOrNo('Help improve Amplify CLI by sharing non sensitive configurations on failures', false); | ||
DebugConfig.Instance.setAndWriteShareProject(result); | ||
if (result === false) { | ||
return; | ||
} | ||
const diagnoseAction = await prompter.pick('What would you like to do?', choices); | ||
if (diagnoseAction !== choices[0]) { | ||
return; | ||
} |
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.
We should.
- Double check this condition with
if (!isHeadless && DebugConfig.Instance.promptSendReport()) { - We should undo all the changes made to e2e/migration/other tests. They don't scale. Current implementation covers very narrow failure modes that are expected. All unexpected errors will now result in
nexpect
timing out on unexpected prompt should failure happen (transient or not). This will contribute to overall e2e test runtime. - Instead of that we should.
- Opt out existing e2e tests by default, i.e. make sure
DebugConfig.Instance.promptSendReport()
returnsfalse
, so this prompt is never asked there. - Add test(s) and test utils dedicated to covering that particular scenario.
- Opt out existing e2e tests by default, i.e. make sure
if (diagnoseAction !== choices[0]) { | ||
return; | ||
if (DebugConfig.Instance.promptSendReport()) { | ||
const result = await prompter.yesOrNo('Help improve Amplify CLI by sharing non sensitive configurations on failures', false); |
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.
Looks like we've already got a prompt on line 37, why do we need to add this line to zipSend
again? https://github.com/aws-amplify/amplify-cli/pull/12710/files#diff-71cabcaa3379af3b9458f6802b25c65f6c00768606d5ef7f8c010094d8720aa1R37
Description of changes
Previously the CLI would ask permission for diagnose command on init but this PR moves the ask to the beginning of diagnose command. This is set on a per-project basis.
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.