Added a different folder per operation to test that each command (or function) needs to download the ECS schema again.

To be removed.

These tests do not require any ecs schema URL because there is no external field to be resolved.

but using the constructor function wont make any difference? i mean, we can have X string defined as url, if the are no external fields, does it fail? if is defined? even with default? 🤔

we can have X string defined as url, if the are no external fields, does it fail?

If there are no external fields defined in the package, elastic-package is not going to try to download any ECS schema.

About the other questions, I think there would not be any issue since the methods to get the ECS base URL take into account the scenario where the string is empty

https://github.com/elastic/elastic-package/pull/3220/changes#diff-87c6c2397b75252e87ef19b597b96a3973f47f86588782fd693b64ab3de079faR69-R75

Is that what you were referring to ?

While testing this PR, I thought that there could be scenarios where it is going to be difficult to detect the missing fields.SchemaURLs parameter in our code base.

One example about this is an execution that requires to check or download in two different locations the ECS schema of a given version. Let's consider:

1. When building and installing the package from a folder it requires ECS schema v9.1
   • This code has added the required fields.SchemaURLs from the configuration, and it downloads the ECS schema v9.1 to the cache folder.
2. When running system tests, it is required to render all fields again and therefore it is required to check/download the ECS schema:
   • Assumption: This code does not add the WithSchemaURLs(urls) in the CreateValidator function .
   • If the previous step was not executed, this would have failed since the ECS schema is not present in the host, and the ECSBase URL is an empty string.
   • However, as the ECS schema was downloaded in the first step, it makes use of it. It does not require to use the ECSBase URL and it does not fail.

Any ideas about this? @elastic/ecosystem

this is covered on my comment https://github.com/elastic/elastic-package/pull/3220/changes#r2720765211 :D

I would need a second point of view here @elastic/ecosystem.

These are the options I can think of:

1. Keep it as it is currently implemented in this PR. Thereby, if there is no urls.ECSBase defined (it is empty), elastic-package is going to use the default URL.

   • This is the same behavior as in the configuration file, if the given key is not defined.
   • That would help in those usages where the fields.SchemaURLs is not added to the corresponding functions/structs (e.g. FleetPackage, CreateValidator...) in the source code that by default contain an empty definition. If the developers forget to add this field via the required options, fields or parameters, thanks to this check (if) the code will always try to use the default URL. And therefore, this would help to avoid errors in runtime.

2. The other option is just removing this check (if) condition, and fail if there is no ECSBase URL defined.

   • Doing so, elastic-package will fail just in those scenarios where the ECS schema is not present locally in the host and it tries to download it.

WDYT ??

too :D https://github.com/elastic/elastic-package/pull/3220/changes#r2720765211

This variable in #3114 was defined in internal/install/application_configuration.go, but being defined caused some cycle import errors when working on this branch.

I thought here as part of the fields package could also be a good location for that default variable. WDYT ?

Looking into the code, I see that…

DefaultECSSchemaBaseURL is used at the config (install) package to set the value as an initial value, so using the config value of this will never be empty.

When the value config.SchemaURLs is assigned to the fields.SchemaURLs, this is either the default or the user configuration, so not empty. It’s only used empty, and with the need of assigning later a default value, when using it for tests…

With that said, what about making fieldsSchemaURLs an entity of its own and using a constructor fields.NewSchemaURLs(value) which returns fields.SchemaURLs{} initiated to either the value, or if it is empty, the default one. 

For grabbing the value, create a method Get() that will return the set value of the private property of fields.SchemaURLs. 

This way, default value setting is centralized where the value is used, instead of when the config is read, decoupling the default value from the configuration reading. If no config is set by the user, then this value (from my POV) makes sense that it is empty, and resolve this case when the value is going to be used (fields management).

TL;DR; something like this

dependency_manager.go

type SchemaURLs struct {
    ecsBase string
}

func NewSchemaURLs(v string) SchemaURLs {
    value := DefaultECSSchemaBaseURL
    if v != "" {
        value = v
    }
    return SchemaURLs{
        ecsBase: value,
    }
}

func (s SchemaURLs) ECSBase() string {
    return s.ecsBase
}

application_configuration.go

type schemaURLsConfig struct {
    ECSBase string `yaml:"ecs_base"`
}

type configFile struct {
    Stack   stack `yaml:"stack"`
    Profile struct {
        Current string `yaml:"current"`
    } `yaml:"profile"`
    SchemaURLs schemaURLsConfig `yaml:"schema_urls"`
}

// SchemaURLs returns the URLs used to retrieve schemas.
func (ac *ApplicationConfiguration) SchemaURLsECS() string {
    return ac.c.SchemaURLs.ECSBase
}

build.go

target, err := builder.BuildPackage(builder.BuildOptions{
        PackageRoot:    packageRoot,
        BuildDir:       buildDir,
        CreateZip:      createZip,
        SignPackage:    signPackage,
        SkipValidation: skipValidation,
        RepositoryRoot: repositoryRoot,
        UpdateReadmes:  true,
        SchemaURLs:     fields.NewSchemaURLs(config.SchemaURLsECS()),
    })

DefaultECSSchemaBaseURL is used at the config (install) package to set the value as an initial value, so using the config value of this will never be empty.

In the current implementation , while developing new features it could be missing to add some fields.SchemaURLs to some function or struct. That would cause that ECSBase field would be an empty string. If that happens, there could be errors when trying to download the ECS schema if it is not present in the host (~/.elastic-package/cache/fields/ecs/...).

Related to this comment: #3220

With that said, what about making fieldsSchemaURLs an entity of its own and using a constructor fields.NewSchemaURLs(value) which returns fields.SchemaURLs{} initiated to either the value, or if it is empty, the default one. 

For grabbing the value, create a method Get() that will return the set value of the private property of fields.SchemaURLs. 

This way, default value setting is centralized where the value is used, instead of when the config is read, decoupling the default value from the configuration reading. If no config is set by the user, then this value (from my POV) makes sense that it is empty, and resolve this case when the value is going to be used (fields management).

I'll take a look at this approach and try to make it work to ensure that always the ECSBase field from the struct is always set. Thanks!

In the last commits pushed, I updated the code to ensure that it is always the default value used (even when reading from YAML). Field containing the url has been set to be private ecsBase and it would be available just via a method from struct.

Doing so, it will help us to ensure that for every scenario it can be set the default value.

Added to avoid panic error here.

Nit. In #3232 appConfig is used as name for this variable. Should we use the same name in both places for consistency?