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

License nagging #1455

Merged
merged 12 commits into from
Sep 26, 2024
Merged

License nagging #1455

merged 12 commits into from
Sep 26, 2024

Conversation

weeco
Copy link
Contributor

@weeco weeco commented Sep 21, 2024

This pull request changes:

  1. Add new ListEnterpriseFeatures RPC to console pkg (may return CodeUnimplemented if unreachable or unimplemented)
  2. Add violation bool in ListLicenses response (defaults to false if admin api endpoint is unreachable or unimplemented)

Has a dependency on: redpanda-data/common-go#24

Copy link

github-actions bot commented Sep 21, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / push-module (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 26, 2024, 3:14 PM

@weeco weeco marked this pull request as ready for review September 24, 2024 12:18
enterpriseFeatures, err := s.adminapiCl.GetEnterpriseFeatures(ctx)
if err == nil {
isViolation = enterpriseFeatures.Violation
}
Copy link
Member

Choose a reason for hiding this comment

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

if GetEnterpriseFeatures fails we default to "no violation"... is that intentional?
Couldn't just someone turn off / ensure this endpoint fails and always have a valid license?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is intentional, since we will also be dealing with Redpanda clusters that do not yet implement this new endpoint. It is not a problem, because this is just some soft nagging / reminding the users that they are violating the license. If we are not certain they are violating the license we better be quiet was the idea here

Comment on lines 1574 to 1578
this.enterpriseFeaturesUsed = enterpriseFeaturesResponse.features;

// Handle the second response
this.licenses = licensesResponse.licenses;
this.licenseViolation = licensesResponse.violation;
Copy link
Contributor

Choose a reason for hiding this comment

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

what if enterpriseFeaturesResponse is of type Errorr? which can happen because ofthe catch and "return err" in line 1567

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rikimaru0345 can you please check now?

@jvorcak
Copy link
Collaborator

jvorcak commented Sep 26, 2024

Screenshot 2024-09-26 at 16 12 59 Screenshot 2024-09-26 at 16 12 56 Screenshot 2024-09-26 at 16 12 53 Screenshot 2024-09-26 at 16 12 46 Screenshot 2024-09-26 at 16 12 43 Screenshot 2024-09-26 at 16 12 31 Failed state Screenshot 2024-09-26 at 16 14 26

@ivpanda
Copy link

ivpanda commented Sep 26, 2024

LGTM! Just two small comments on the errors, if we could show them using our error components, like this:

On the Admin page:

image

In the Details box:

image

Thanks!

@jvorcak
Copy link
Collaborator

jvorcak commented Sep 26, 2024

LGTM! Just two small comments on the errors, if we could show them using our error components, like this:

On the Admin page:

image ### In the Details box: image Thanks!

I'd try to avoid showing "... or upload a new license" WDYT?
In my opinion it somehow might imply that there is a problem with a configuration and when I upload a new license it can fade away. But this error is shown in a corner case like network error or similar that are irrelevant to the actual configuration.

@jvorcak
Copy link
Collaborator

jvorcak commented Sep 26, 2024

Screenshot 2024-09-26 at 17 13 25
Screenshot 2024-09-26 at 17 08 06

@jvorcak jvorcak merged commit 77e4e73 into master Sep 26, 2024
8 checks passed
@jvorcak jvorcak deleted the license-nagging branch September 26, 2024 18:11
@mattschumpert
Copy link

mattschumpert commented Sep 26, 2024

@jvorcak
Right, in that case just 'failed to load license info'. If the license key itself was found to be invalid, I assume that would fail differently, in the upload page.

Also,

I am confused why we still see different license status in license details. Uploading an Enterprise license is supposed to also push the license to the Redpanda cluster (what now says 'Core Community'). This part should should say 'Repanda Community Edition' or 'Redpanda Enterprise Edition', and after upload of a license at a minimum both should be 'Enterprise'. The Console one should be 'Console Community Edition', 'Console Enterprise Edition.

Also, a header for 'Expiry Date' column would be nice.

@jvorcak
Copy link
Collaborator

jvorcak commented Sep 29, 2024

@mattschumpert I've addressed the details we talked about in #1464

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

Successfully merging this pull request may close these issues.

6 participants