internal/elasticsearch/ingest: export LoadIngestPipelineFiles, InstallPipelinesInElasticsearch and GetPipelineNameWithNonce

[efd6]

For #2868

Please take a look.

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: d9ec1c6

History

• 💔 Build #6203 failed 9eea668

cc @efd6

[jsoriano]

The nonce was an implementation detail to have unique names, I wonder if this should be exposed.

I would prefer to expose an interface like the following one:

// LoadIngestPipelineFiles returns the set of pipelines found in the directory
// elasticsearch/ingest_pipeline under the provided data stream path.
LoadIngestPipelineFiles(dataStreamPath string) ([]Pipeline, error)

// GenerateUniquePipelineNames decorates pipeline names with a suffix
// to ensure they are unique.
GenerateUniquePipelineNames([]Pipeline) []Pipeline

// InstallPipelines installs the provided pipelines into the
// Elasticsearch instance specified by the provided API handle. 
InstallPipelines(ctx context.Context, api *elasticsearch.API, pipelines []Pipeline) error

This way the load and install methods can be used for any case where we need to load or install pipelines, and in the cases where we need to decorate them, we can do it with the other method.

WDYT?

[jsoriano]

Why is this method needed out of this package? The pipelines returned by LoadIngestPipelineFiles already include the nonce 🤔

[efd6]

The nonce is something that ideally I'd like not to exist, but it needs to. Separating them allows me to (mostly) ignore it.

All of these packages are in internal, so I don't think we need to be precious about exporting labels.

[jsoriano]

I think we could take the opportunity to improve the interface so callers don't need to think about "nonces", they would only need to think if they need unique names or not, what seems like a higher level concept more aligned with the actual needing.

But well, I guess this is something we can refactor later, so chose the option you prefer.

[efd6]

I think there is a misunderstanding of the reasons that I'm making these changes. To clarify, I'm making it possible to write scripted testing for elastic fleet packages, I am not making it possible for third-party code to consume functions that are exported by the packages I'm touching (this would not be possible without significantly larger changes since all the packages here are in internal and so cannot be imported by packages outside of github.com/elastic/elastic-package. The changes here are the minimal change required for me to be able to script tests; the API proposed would be both a greater change and would be less ergonomic for my use.

[jsoriano]

I am not making it possible for third-party code to consume functions that are exported by the packages I'm touching

That's clear. But even in this case it is useful to have limited interfaces and responsibilities in packages. This helps with testing, and with having cleaner interfaces between components in general.

Now that we are touching these functions we could take the opportunity to improve their interfaces so they don't expose internal details to other packages, so callers don't need to care about them. But well, I guess we can also refactor later.

[jsoriano]

I think it would be better to avoid exposing the "nonce", but let's go on if you think current interface is better for your use. Not a strong opinion on my suggestion.

[efd6]

Thanks