Allow to define EPR and Kibana repository URL in elastic-package configuration file#3232
Conversation
|
Missing to add a new profile setting |
| profile: | ||
| current: default | ||
| schema_urls: | ||
| ecs_base: https://raw.githubusercontent.com/elastic/ecs |
There was a problem hiding this comment.
nit: just thinking now, should this be ecs_base_url ??
There was a problem hiding this comment.
I think it would not be needed since this field is under the key schema_urls that already refers to URLs.
| } | ||
| p.elasticsearch = elasticsearch | ||
|
|
||
| p.registry = registry.NewClient(appConfig.PackageRegistryBaseURL()) |
There was a problem hiding this comment.
could we benefit of passing just the url as a string? instead of the struct for appConfig, given that we only need one of its params
| Profile *profile.Profile | ||
| Printer Printer | ||
| Profile *profile.Profile | ||
| AppConfig *install.ApplicationConfiguration |
There was a problem hiding this comment.
currently we use this AppConfig to provide the registryURL, could we just pass along the string url, and change it into a struct if we need future options?
There was a problem hiding this comment.
This value could depend on the specific use case.
For instance, for stack commands the registry URL value will depend on where it is used:
To write the value of EPR_PROXY_TO variable, this value depends on the stack.epr.proxy_to setting from the profile or the value in the configuration file.
However other usages in stack commands will depend on another profile setting stack.epr.base_url and the value in the configuration file. This will be required when doing elastic-package stack up in Serverless or Environment providers.
This last scenario described can be observed in the changes applied in #3235
So, maybe we need to keep both the profile and the configuration.
jsoriano
left a comment
There was a problem hiding this comment.
Looks good, thanks, added some small suggestions.
cmd/status.go
Outdated
| } | ||
|
|
||
| func getPackageStatus(packageName string, options registry.SearchOptions) (*status.PackageStatus, error) { | ||
| func getPackageStatus(packageName string, options registry.SearchOptions, registryClient *registry.Client) (*status.PackageStatus, error) { |
There was a problem hiding this comment.
Nit. I would place the client as first parameter, here and in the other methods where this is added.
| func getPackageStatus(packageName string, options registry.SearchOptions, registryClient *registry.Client) (*status.PackageStatus, error) { | |
| func getPackageStatus(registryClient *registry.Client, packageName string, options registry.SearchOptions) (*status.PackageStatus, error) { |
There was a problem hiding this comment.
Sure! I've applied these changes in 9108ad8
| if ac == nil { | ||
| return "https://raw.githubusercontent.com/elastic/kibana" | ||
| } | ||
| if ac.c.Status.KibanaRepository.BaseURL != "" { | ||
| return ac.c.Status.KibanaRepository.BaseURL | ||
| } | ||
| return defaultKibanaRepositoryBaseURL |
There was a problem hiding this comment.
Or:
| if ac == nil { | |
| return "https://raw.githubusercontent.com/elastic/kibana" | |
| } | |
| if ac.c.Status.KibanaRepository.BaseURL != "" { | |
| return ac.c.Status.KibanaRepository.BaseURL | |
| } | |
| return defaultKibanaRepositoryBaseURL | |
| if ac == nil || ac.c.Status.KibanaRepository.BaseURL == "" { | |
| return defaultKibanaRepositoryBaseURL | |
| } | |
| return ac.c.Status.KibanaRepository.BaseURL |
| require.NoError(t, err) | ||
| } | ||
|
|
||
| config, err := Configuration() |
There was a problem hiding this comment.
Perhaps we could have a method that initializes the configuration from a given directory, so we don't need to set ELASTIC_PACKAGE_DATA_HOME.
| config, err := Configuration() | |
| config, err := configurationFromDir(tmpDir) |
💛 Build succeeded, but was flaky
Failed CI StepsHistory
cc @mrodm |
Follows #3229
Part of #2993
Includes two new configuration into
~/.elastic-package/config.ymlto allow overriding the defaults URLs used by elastic-package.Example:
This PR keeps the same default values:
In the case of the package registry, the value from the configuration file will be taken into consideration for the following commands such as:
stackUpfunction)This PR also takes into account the profile setting
stack.epr.proxy_to. This configuration, if set, will take precedence to get the value to fill the value for the EPR_PROXY_TO environment variable in the packge-registry Dockerfile.Author's checklist
How to test this PR locally
~/.elastic-package/config.ymlincluding:elastic-package statuswith different parameters:environmentprovider: