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

Skip error uninstalling system package #1315

Merged
merged 9 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions internal/testrunner/runners/asset/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (r runner) CanRunPerDataStream() bool {
}

// Run runs the asset loading tests
func (r runner) Run(options testrunner.TestOptions) ([]testrunner.TestResult, error) {
func (r *runner) Run(options testrunner.TestOptions) ([]testrunner.TestResult, error) {
r.testFolder = options.TestFolder
r.packageRootPath = options.PackageRootPath

Expand Down Expand Up @@ -82,7 +82,11 @@ func (r *runner) run() ([]testrunner.TestResult, error) {
if err != nil {
return result.WithError(errors.Wrap(err, "could not create kibana client"))
}
packageInstaller, err := installer.CreateForManifest(kibanaClient, r.packageRootPath)
packageInstaller, err := installer.NewForPackage(installer.Options{
Kibana: kibanaClient,
RootPath: r.packageRootPath,
SkipValidation: true,
})
if err != nil {
return result.WithError(errors.Wrap(err, "can't create the package installer"))
}
Expand All @@ -92,10 +96,25 @@ func (r *runner) run() ([]testrunner.TestResult, error) {
}

r.removePackageHandler = func() error {
pkgManifest, err := packages.ReadPackageManifestFromPackageRoot(r.packageRootPath)
if err != nil {
return fmt.Errorf("reading package manifest failed: %w", err)
}

logger.Debug("removing package...")
if err := packageInstaller.Uninstall(); err != nil {
return errors.Wrap(err, "error cleaning up package")
err = packageInstaller.Uninstall()
if err == nil {
return nil
}
// by default system package is part of an agent policy and it cannot be uninstalled
// https://github.com/elastic/elastic-package/blob/5f65dc29811c57454bc7142aaf73725b6d4dc8e6/internal/stack/_static/kibana.yml.tmpl#L62
switch pkgManifest.Name {
case "system":
logger.Debugf("failed to uninstall package %q: %s", pkgManifest.Name, err.Error())
default:
logger.Warnf("failed to uninstall package %q: %s", pkgManifest.Name, err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

It may be also good to add a comment here about why we are only logging the error and not returning it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment to explain it 6e0af5e

}

return nil
}

Expand Down
19 changes: 17 additions & 2 deletions internal/testrunner/runners/system/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,25 @@ func (r *runner) runTest(config *testConfig, ctxt servicedeployer.ServiceContext
}
r.deletePackageHandler = func() error {
err := installer.Uninstall()
if err != nil {
return fmt.Errorf("failed to uninstall package: %v", err)
if err == nil {
return nil
}
// by default system package is part of an agent policy and it cannot be uninstalled
// https://github.com/elastic/elastic-package/blob/5f65dc29811c57454bc7142aaf73725b6d4dc8e6/internal/stack/_static/kibana.yml.tmpl#L62
switch pkgManifest.Name {
case "system":
logger.Debugf("failed to uninstall package %q: %s", pkgManifest.Name, err.Error())
default:
logger.Warnf("failed to uninstall package %q: %s", pkgManifest.Name, err.Error())
}
Copy link
Contributor Author

@mrodm mrodm Jun 20, 2023

Choose a reason for hiding this comment

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

Should we also show a Debug message in case system package fails ? It will always fail. Same for asset runner.

If not this code could be just changed to be something like:

		// by default system package is part of an agent policy and it cannot be uninstalled
		// https://github.com/elastic/elastic-package/blob/5f65dc29811c57454bc7142aaf73725b6d4dc8e6/internal/stack/_static/kibana.yml.tmpl#L62
		if err != nil && pkgManifest.Name != "system" {
			logger.Warnf("failed to uninstall package %q: %s", pkgManifest.Name, err.Error())
		}
		return nil

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's log the error only for other packages.


return nil
// by default system package is part of an agent policy and it cannot be uninstalled
// https://github.com/elastic/elastic-package/blob/5f65dc29811c57454bc7142aaf73725b6d4dc8e6/internal/stack/_static/kibana.yml.tmpl#L62
// if err != nil && pkgManifest.Name != "system" {
// logger.Warnf("failed to uninstall package %q: %s", pkgManifest.Name, err.Error())
// }
// return nil
}

// Configure package (single data stream) via Ingest Manager APIs.
Expand Down
2 changes: 1 addition & 1 deletion internal/testrunner/testrunner.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ func Run(testType TestType, options TestOptions) ([]TestResult, error) {
return nil, errors.Wrap(err, "could not complete test run")
}
if tdErr != nil {
return results, errors.Wrap(err, "could not teardown test runner")
return results, errors.Wrap(tdErr, "could not teardown test runner")
}
return results, nil
}
Expand Down
1 change: 1 addition & 0 deletions scripts/links_table.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
links:
elastic-main: "https://www.elastic.co/guide"
getting-started-observability: "https://www.elastic.co/guide/en/welcome-to-elastic/current/getting-started-observability.html"
3 changes: 3 additions & 0 deletions test/packages/parallel/system/_dev/build/build.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
dependencies:
ecs:
reference: [email protected]
Loading