Skip to content
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: adding cta for docker users #1352

Merged
merged 1 commit into from
Aug 24, 2020
Merged

Conversation

RotemS
Copy link
Contributor

@RotemS RotemS commented Aug 19, 2020

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Adding a CTA if a test is made without an API key and with a Docker JWT.
There are 2 cases -

  • Scan successful
    image
    image

  • Scan not successful - the user has reached the free test limit
    image

How should this be manually tested?

Need to setup the env locally with JWT auth enabled and with a valid token.
In any snyk test where the API key doesn't exist and the SNYK_DOCKER_TOKEN env var is set, the CTA should be displayed.

@RotemS RotemS self-assigned this Aug 19, 2020
@RotemS RotemS force-pushed the DC-789/prompt-docker-user-signup branch 3 times, most recently from 956692b to 8a1c76f Compare August 20, 2020 08:11
@RotemS RotemS marked this pull request as ready for review August 20, 2020 09:35
@RotemS RotemS requested review from a team as code owners August 20, 2020 09:35
@ghost ghost requested review from ekbsnyk and MegaBean August 20, 2020 09:35
Copy link
Contributor

@hisenb3rg hisenb3rg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@@ -263,6 +264,10 @@ async function test(...args: MethodArgs): Promise<TestCommandResult> {
// first one
error.code = errorResults[0].code;
error.userMessage = errorResults[0].userMessage;
if (error.userMessage === 'Test limit reached!' && options.isDockerUser) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That condition might be a bit fragile (if message changes), but probably we don't have an alternative check we could use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any better way of doing this, but open to suggestions if anyone would have an idea

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a type to the error? Maybe do something like error.userMessage === SomeDefinedError.errorMessage

Defined error example: https://github.com/snyk/snyk/blob/924f10abbc8d78da74fcd5ee0fbdd14dc11e7705/src/lib/errors/authentication-failed-error.ts#L5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orsagie WDYT? added an error type for it and changed the condition

@RotemS RotemS force-pushed the DC-789/prompt-docker-user-signup branch 2 times, most recently from 983dfea to e177956 Compare August 23, 2020 13:07
@RotemS RotemS force-pushed the DC-789/prompt-docker-user-signup branch from e177956 to 4f16863 Compare August 23, 2020 13:12
@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2020

Expected release notes (by @RotemS)

features:
adding cta for docker users (4f16863)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

Copy link
Contributor

@orsagie orsagie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename the Pull request to not have the internal ticket ID before merging?

@RotemS RotemS changed the title [DC-789] feat: adding cta for docker users feat: adding cta for docker users Aug 23, 2020
@RotemS RotemS merged commit 83adf4f into master Aug 24, 2020
@RotemS RotemS deleted the DC-789/prompt-docker-user-signup branch August 24, 2020 09:18
@snyksec
Copy link

snyksec commented Aug 24, 2020

🎉 This PR is included in version 1.382.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants