Support custom packages path #525
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
|
There was a problem hiding this comment.
Change LGTM. We can still iterated on my comments below later.
One thing I don't like too much is that the multiple paths add complexity in too many different places but I'm sure we find ways moving forward to simplify / unify it.
Also as a follow up, it could be worth adding a test case for the multi path support so we don't break it.
config.docker.yml
Outdated
| @@ -0,0 +1,7 @@ | |||
| public_dir: ./public | |||
| packages_paths: | |||
There was a problem hiding this comment.
I'm torn if we should call it package_paths?
| ) | ||
|
|
||
| func Build() error { | ||
| packagePathsEnv := os.Getenv("PACKAGE_PATHS") |
There was a problem hiding this comment.
It would be nice to support this in the future directly in the registry to pass in an env variable for this.
There was a problem hiding this comment.
We can add it in the future, but the goal is to use the config file. If you prefer, we can create a new issue to read same configuration from environment variables, but not sure if there is a specific use case. In all cases I'm aware you can depend on the config.yml file.
| @@ -37,7 +38,7 @@ var ( | |||
| configPath = "config.yml" | |||
|
|
|||
| defaultConfig = Config{ | |||
| PublicDir: "public", | |||
| PublicDir: "./public", // left for legacy purposes | |||
There was a problem hiding this comment.
Curious which legacy part you are referring to here? Do we need it somewhere?
There was a problem hiding this comment.
Every piece of software that mounts directory for packages, e.g. all testing environments where package path is mounted as /public.
There was a problem hiding this comment.
I assume we should update these accordingly?
There was a problem hiding this comment.
No, it's for legacy purposes. Thanks for this line, I will work for configuration files defining /public directory.
The package directory is appended here: https://github.com/elastic/package-registry/blob/master/main.go#L126
There was a problem hiding this comment.
I think I miss what legacy purpose means in this context. All the registries and builds which are run are currently under our control so we can get rid of the public path there and remove it. But I assume I miss something?
My goal is that as soon as we hit 7.9, we don't have legacy code inside as we will build up more then enough in the future.
There was a problem hiding this comment.
It means that we need to remove all references to public from these places:
https://github.com/elastic/integrations/blob/master/testing/environments/snapshot.yml#L47
https://github.com/elastic/package-storage/blob/master/testing/environments/snapshot.yml#L43
and notify people to update their workplaces, because the package-registry will not support this anymore.
There was a problem hiding this comment.
I will open PR's to adjust these accordingly. I expect that all devs building packages at the moment update to the most recent changes at least with the updates of the snapshot build.
There was a problem hiding this comment.
I found the missing bit on my end. We don't provide any other default at the moment. My proposal would be to use ./packages as default?
There was a problem hiding this comment.
I opened #551 as an alternative proposal.
This PR add support for multiple paths with packages.
Issue: #505