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

[CI] Enable wolfi Elastic Agent images in tests #2067

Merged
merged 22 commits into from
Sep 3, 2024

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Sep 2, 2024

Run tests using Elastic Agent images where it is possible.

Author's checklist

@mrodm mrodm self-assigned this Sep 2, 2024
@@ -17,6 +17,7 @@ inputs:
- apache-access
type: logfile
use_output: default
namespaces: []
Copy link
Contributor Author

@mrodm mrodm Sep 3, 2024

Choose a reason for hiding this comment

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

It looks like that for policy tests it should also be needed to be able to mark some fields optional @jsoriano

This namespaces field appeared when testing with 8.16.0-SNAPSHOT. But they are not present in older stack versions (e.g. 8.15.0).

Changes required present in these two commits:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting these two commits in order to be able to merge this PR.

Comment on lines +58 to +60
// Try to keep the test agnostic from the environment variables defined in CI
t.Setenv(disableElasticAgentWolfiEnvVar, "")
os.Unsetenv(disableElasticAgentWolfiEnvVar)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this cause any issues ?
It looks like the environment variable is defined in other tests even here it is unset.

Copy link
Member

Choose a reason for hiding this comment

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

Setting environment variables is only going to be problematic if tests are run in parallel, as it affects the global process. t.Setenv recovers the previous value on cleanup, see https://pkg.go.dev/testing#B.Setenv.

Copy link
Contributor Author

@mrodm mrodm Sep 3, 2024

Choose a reason for hiding this comment

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

Perfect, so as these tests do not use t.Parallel(), I guess it is safe here. Even if it is called os.Unsetenv , the environment variable would be recovered aftwards thanks to the t.Setenv()

t.Parallel() is used in other tests:

  • internal/servicedeployer/terraform_test.go
  • internal/testrunner/runners/system/tester_test.go

Copy link
Member

Choose a reason for hiding this comment

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

I think each package is executed in a different process, so we should be fine if the tests are in different packages.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 3, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

History

cc @mrodm

@mrodm mrodm marked this pull request as ready for review September 3, 2024 15:59
@mrodm mrodm requested a review from a team as a code owner September 3, 2024 15:59
@mrodm mrodm requested a review from jsoriano September 3, 2024 16:19
@mrodm mrodm merged commit 19b2d35 into elastic:main Sep 3, 2024
3 checks passed
@mrodm mrodm deleted the enable-wolfi-images-tests branch September 3, 2024 16:42
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