-
Notifications
You must be signed in to change notification settings - Fork 717
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
Use enterprise_trial as the trial license type #2934
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.
Good catch. I'm +1 for 1.1.0.
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
var operatorNamespace string | ||
flag.StringVar(&operatorNamespace, "operator-namespace", "elastic-system", "indicates the namespace where the operator is deployed") | ||
flag.Parse() | ||
licensingInfo, err := license.NewResourceReporter(newK8sClient(), operatorNamespace).Get() |
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.
I am not sure we need still need this tool.
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.
Agreed, maybe open an issue to revisit keeping it in there? I'm not sure how valuable it is
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.
I left it because it was handy for testing the license reporting and in case someone wanted to get it in real time but it is not documented. So I don't really see any good reason to keep it.
c := k8s.WrapClient(client) | ||
return ResourceReporter{ | ||
aggregator: Aggregator{ | ||
client: c, | ||
}, | ||
licensingResolver: LicensingResolver{ | ||
client: c, | ||
client: c, | ||
operatorNs: operatorNs, |
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.
This was always "" up until now, which lead to errors now that we actually do trial license checks.
return l.License.Type == LicenseTypeEnterpriseTrial || l.License.Type == LicenseTypeLegacyTrial | ||
} | ||
|
||
// IsECKManagedTrial returns true if this license has been generated by trial controller or is eligible to be turned into one. |
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.
Nit: "or is eligible to be turned into one." is kind of confusing, but I'm not sure what would be better. Maybe "or is eligible to have its license generated by the trial controller" but that still reads weird. Maybe just "or is eligible to be"? All sound a little bit off
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
* Introduce enterprise_trial, keep enterprise-trial for backwards compatibility * Fix license checker and add tests around the new license type * Mention trial extensions in licensing doc and use the new trial type identifier * Make sure license reporter's operator namespace is always initialised
* Introduce enterprise_trial, keep enterprise-trial for backwards compatibility * Fix license checker and add tests around the new license type * Mention trial extensions in licensing doc and use the new trial type identifier * Make sure license reporter's operator namespace is always initialised
Fixes #2915
enterprise_trial
as the correct license type for trial licenses both auto-generated ones as well as externally generated onesenterprise-trial
for backwards compatibility in auto-generated trial licensesLabeled 1.1.0, but open for discussion. Happy to spin off a smaller PR if desired for 1.1.0