-
Notifications
You must be signed in to change notification settings - Fork 311
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
handle 423 api error a trial license expiration #3929
Conversation
Thank you for your contribution. unfortunately, one or more of your commits are missing the required "Signed-off-by:" statement. Signing off is part of the Developer Certificate of Origin (DCO) which is used by this project. Read the DCO and project contributing guide carefully, and amend your commits using the git CLI. Note that this does not require any cryptography, keys or special steps to be taken. 💡 Shall we fix this?This will only take a few moments. First, clone your fork and checkout this branch using the git CLI. Next, set up your real name and email address:
Finally, run one of these commands to add the "Signed-off-by" line to your commits. If you only have one commit so far then run: Check that the message has been added properly by running "git log". |
1 similar comment
Thank you for your contribution. unfortunately, one or more of your commits are missing the required "Signed-off-by:" statement. Signing off is part of the Developer Certificate of Origin (DCO) which is used by this project. Read the DCO and project contributing guide carefully, and amend your commits using the git CLI. Note that this does not require any cryptography, keys or special steps to be taken. 💡 Shall we fix this?This will only take a few moments. First, clone your fork and checkout this branch using the git CLI. Next, set up your real name and email address:
Finally, run one of these commands to add the "Signed-off-by" line to your commits. If you only have one commit so far then run: Check that the message has been added properly by running "git log". |
Thank you for your contribution. unfortunately, one or more of your commits are missing the required "Signed-off-by:" statement. Signing off is part of the Developer Certificate of Origin (DCO) which is used by this project. Read the DCO and project contributing guide carefully, and amend your commits using the git CLI. Note that this does not require any cryptography, keys or special steps to be taken. 💡 Shall we fix this?This will only take a few moments. First, clone your fork and checkout this branch using the git CLI. Next, set up your real name and email address:
Finally, run one of these commands to add the "Signed-off-by" line to your commits. If you only have one commit so far then run: Check that the message has been added properly by running "git log". |
@maroshii Could you provide some screenshot on how the error looks like when it's triggered, please? |
@maroshii How can I modify my license to simulate an expiration and get this error? |
@ifbyol this is how it looks Copy was taken from the epic |
@andreafalzetti Backend part is not yet ready, still WIP. The CLI<>Backend contract will be that whenever the backend returns a 423 the CLI will understand it as a license expired event. To reproduce this you can run the backend and add a middleware that returns 423 for all requests |
@@ -258,6 +264,10 @@ func translateAPIErr(err error) error { | |||
|
|||
} | |||
|
|||
func isAPITrialExpiredError(err error) bool { | |||
return strings.HasPrefix(err.Error(), "non-200 OK status code: 423") |
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.
Are we sure we will return the error like this?
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.
Yes. It is awful but it is the exact prefix as returned by the library: https://github.com/shurcooL/graphql/blob/master/graphql.go#L79. For other errors we are just searching for a specific string but I think that is much more error prone
pkg/okteto/client.go
Outdated
case isAPITrialExpiredError(err): | ||
return oktetoErrors.UserError{ | ||
E: oktetoErrors.ErrTrialExpired, | ||
Hint: "This command-line tool will no longer work. Please contact our sales team (sales@okteto.com) to obtain a new license key", |
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.
it might not be confusing for the user to say This command-line tool will no longer work.
?. For vanilla clusters the tool would still work
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.
Maybe the copy should mention "for this context" or "for this instance" as the CLI needs to log to some context, also for cloud will still work i guess
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.
Yeah, I think that's a good callout @AdrianPedriza. We might need to reword this cc @ifbyol
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.
@omnomagonz this is related to the error message we talked about to make it a more generic. What are your thoughts about the point raised here?
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.
From this Slack thread, I believe we landed on:
The Okteto instance for your current context has an expired or missing license. Please contact your administrator for more information.
162061c
to
a3d37fc
Compare
Proposed changes
Gracefully handle 423 Locked return status from the api. Once an okteto trial license has expired, the okteto API will return an http
423 Locked
status code indicating that the instance is locked. As part of this PR, we now handle this returns status and show the appropriate message.Note that this change is backwards compatible, doesn't require any specific backend version and can be merge at any time.