Skip to content

Conversation

@jsoriano
Copy link
Member

This will allow to earlier detect if one is not testing the version they currently have in the working copy.

From 8.7.0 version, it builds and install the zip package. On older versions it installs whatever is in the configured package registry. In that case, if the version of the package is not available in the repository, test execution will fail.

Also removes uses of errors.Wrap in the file.

It won't be tested in CI till stack version is updated to >= 8.7.0.

This will allow to earlier detect if one is not testing the version they
currently have in the working copy.

From 8.7.0 version, it builds and install the zip package. On older
versions it installs whatever is in the configured package registry. In
that case, if the version of the package is not available in the
repository, test execution will fail.
@jsoriano jsoriano requested a review from a team May 26, 2023 08:59
@jsoriano jsoriano self-assigned this May 26, 2023
return agents, nil
}

func installPackage(client *kibana.Client, manifest *packages.PackageManifest, rootPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks similar to the factory created in cmd/install.go file. Do you think it would be possible to reuse some of the code? Maybe the installer code:

  • for local packages: internal/packages/installer/installer.go
  • for zip packages: internal/packages/installer/zip_installer.go

https://github.com/elastic/elastic-package/tree/main/internal/packages/installer

Copy link
Member Author

Choose a reason for hiding this comment

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

I have merged both codes in a single factory in the installer package, let me know what you think.

I am also passing the kibana client from the callers, I would like to have more explicit initializations of the clients we use. This is something I have WIP to be able to get the settings from the profile in this branch https://github.com/elastic/elastic-package/compare/main...jsoriano:elastic-package:clients-from-profile?expand=1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have merged both codes in a single factory in the installer package, let me know what you think.

It looks perfect! 💪 thanks for doing this change!

I am also passing the kibana client from the callers, I would like to have more explicit initializations of the clients we use. This is something I have WIP to be able to get the settings from the profile in this branch https://github.com/elastic/elastic-package/compare/main...jsoriano:elastic-package:clients-from-profile?expand=1.

Ok! that would also help us to add more tests if the kibana client is set as a parameter 👍

Copy link
Contributor

@mrodm mrodm left a comment

Choose a reason for hiding this comment

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

Thanks! This will make easier testing changes while developing new version of packages. No need to build and restart package-registry container to get the new version of the package, nice!

Just left one minor suggestion for an error message.

return agents, nil
}

func installPackage(client *kibana.Client, manifest *packages.PackageManifest, rootPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have merged both codes in a single factory in the installer package, let me know what you think.

It looks perfect! 💪 thanks for doing this change!

I am also passing the kibana client from the callers, I would like to have more explicit initializations of the clients we use. This is something I have WIP to be able to get the settings from the profile in this branch https://github.com/elastic/elastic-package/compare/main...jsoriano:elastic-package:clients-from-profile?expand=1.

Ok! that would also help us to add more tests if the kibana client is set as a parameter 👍

if err != nil {
return result.WithError(fmt.Errorf("failed to install package: %v", err))
}
r.deletePackageHandler = func() error {
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't just use defer here, I needed to use a handler to play well with the tear downs here.

@jsoriano jsoriano requested a review from mrodm June 5, 2023 17:30
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @jsoriano

Copy link
Contributor

@mrodm mrodm left a comment

Choose a reason for hiding this comment

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

👍

@jsoriano jsoriano merged commit 1623fb2 into elastic:main Jun 6, 2023
@jsoriano jsoriano deleted the system-tests-install-zip branch June 6, 2023 08:36
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.

3 participants