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

Embed static files using embed package #502

Merged
merged 9 commits into from
Sep 6, 2021

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Sep 2, 2021

Several files were included as strings in Go source files. Leverage the embed package to move them to single files.

Also, include a manifest for elastic-agent in the kubernetes test runner, so it doesn't depend on Beats repository for that.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 2, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-09-06T07:57:44.616+0000

  • Duration: 20 min 22 sec

  • Commit: dd2f929

Test stats 🧪

Test Results
Failed 0
Passed 422
Skipped 5
Total 427

Trends 🧪

Image of Build Times

Image of Tests

@jsoriano
Copy link
Member Author

jsoriano commented Sep 2, 2021

Tests failing because of golang/go#44354, opening PR to change linter (#503).

@MichaelKatsoulis
Copy link
Contributor

Also, include a manifest for elastic-agent in the kubernetes test runner, so it doesn't depend on Beats repository for that.

@jsoriano there was an issue to do exactly the opposite as long as we had a fleet-managed agent manifest in upstream beats repo. The point was not to maintain both at the same time.
I am not sure why we want to change that. We just need to patch the manifest a bit to be fit for testing.

@jsoriano
Copy link
Member Author

jsoriano commented Sep 3, 2021

Also, include a manifest for elastic-agent in the kubernetes test runner, so it doesn't depend on Beats repository for that.

@jsoriano there was an issue to do exactly the opposite as long as we had a fleet-managed agent manifest in upstream beats repo. The point was not to maintain both at the same time.
I am not sure why we want to change that. We just need to patch the manifest a bit to be fit for testing.

Downloading the manifest from the beats/elastic-agent repository adds a dependency into elastic-package that may unexpectedly affect the result of tests in integrations. Any change in this file can unexpectedly break elastic-package or the reproducibility of integrations tests. It also adds cognitive load to both, elastic-package and elastic-agent maintainers, that must be aware that any change in this file may end up affecting many other teams. It is also inconsistent with what we do with other runners such as docker compose, where the definition is already part of elastic-package.

elastic-package is meant to be the reference tool for developers of Elastic packages, inside and outside of Elastic. It should be as standalone as possible, and as a test runner, it should give consistent and reproducible results (same result for a given version of elastic-package and the package tested). For that I think that eventually we have to end-up using specific released versions of everything in elastic-package (no more 7.xs or SNAPSHOTs at least in the default behaviour), and introduce all changes in its code, in a controlled way.

A middle-ground solution for this could be to download an specific version from the beats repository (instead of one of a branch as is being done now) and continue doing the transformations here. But then we would miss the advantages of #328, and then I would prefer to directly have the manifest here.

We can explore additional ways of customizing this elastic-agent deployment in kubernetes from the package side. For example to solve the issue that originated #328, maybe another option would have been to add an additional ClusterRole to the manifests included for the integration, giving the additional required permission without having to modify elastic-package.

If we want to test if the latest manifest in the beats repo works well with the latest kubernetes integration, I think that we should do this in an e2e-testing suite.


// newPackageRegistryConfig returns a Managed Config
func newPackageRegistryConfig(_ string, profilePath string) (*simpleFile, error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: empty line

//go:embed _static/docker-compose-stack.yml
var snapshotYml string

// newSnapshotFile returns a Managed Config
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this comment is a bit misleading (not a Managed Config)

@@ -157,75 +153,25 @@ func installElasticAgentInCluster() error {
return nil
}

// downloadElasticAgentManagedYAML will download a url from a path and return the response body.
func downloadElasticAgentManagedYAML(url string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest I'm on the fence with this change as it isn't inlined with idea we had before - have a single place where we store k8s definitions, so that the correct manifest is picked up without any updates in elastic-package's codebase.

Maybe we should discuss possible options here?

@mtojek mtojek self-requested a review September 6, 2021 09:27
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

We discussed and agreed offline to keep a customized copy of Kubernetes manifest here. Ship it!

@jsoriano jsoriano merged commit d969729 into elastic:master Sep 6, 2021
@jsoriano jsoriano deleted the statics-with-embed branch September 6, 2021 11:08
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.

4 participants