-
Notifications
You must be signed in to change notification settings - Fork 121
Add a new validation to use or not upload API to install packages #2511
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
Add a new validation to use or not upload API to install packages #2511
Conversation
Reverted all changes related to get and validate the Stack subscription
@@ -196,7 +196,7 @@ type Info struct { | |||
Version struct { | |||
Number string `json:"number"` | |||
BuildFlavor string `json:"build_flavor"` | |||
} `json:"version` | |||
} `json:"version"` |
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.
Noticed while trying to get and use the Elastic stack subscription.
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 good, only some nitpicking.
Maybe the only problematic thing is that receiving StatusOK
when checking for the installation API should not be expected.
@@ -27,6 +30,47 @@ func (c *Client) InstallPackage(ctx context.Context, name, version string) ([]pa | |||
return processResults("install", statusCode, respBody) | |||
} | |||
|
|||
// EnsureZipPackageCanBeInstalled checks whether or not it can be installed a package using the upload API. | |||
func (c *Client) EnsureZipPackageCanBeInstalled(ctx context.Context) error { |
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 is a bit hacky and would be prone to fail if this API would change, maybe add a comment mentioning that is only intended to be used between 8.7.0 and 8.8.2, and is only safe to be used on these versions.
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.
Added comment here 4da2bd9
if version.LessThan(semver8_7_0) { | ||
return nil, fmt.Errorf("not supported uploading zip packages in Kibana %s (%s required)", version, semver8_7_0) | ||
} | ||
if version.LessThan(semver8_8_2) { | ||
return nil, fmt.Errorf("not supported uploading zip packages in Kibana %s (%s required or Enteprise license)", version, semver8_8_2) | ||
} |
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. Maybe isAllowedInstallationViaApi
could also return the reason, as we are checking with the same versions as 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.
Added a new return parameter that indicates the reason, just in the case there is an expected failure.
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.
Applied here 05e082b
💚 Build Succeeded
History
cc @mrodm |
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 PR checks whether or not the Elastic stack allows to upload packages via API. If it cannot be used the upload API, packages will be installed from the Package Registry.
Until Elastic stack 8.8.2 the license required was
Enterprise
to use that API endpoint. In Elastic stack 8.8.2 onwards, the license required for this feature was changed tobasic
.Relates Kibana PRs:
Relates elastic/integrations#13377
How to test this PR locally