Define build time dependencies #181
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Trends 🧪 |
versions/1/manifest.spec.yml
Outdated
| reference: | ||
| type: string | ||
| description: Source reference | ||
| pattern: 'git@[a-z0-9]{40}' |
There was a problem hiding this comment.
Allow the use of tags too? (Can be extended in the future in any case)
There was a problem hiding this comment.
The reason why I chose a commit hash is because it's always the same for all develops. If we put a tag here (e.g. [email protected]), the elastic-package will build a slightly different package based on the commit marked with the tag. I was thinking about a switch for elastic-package build, for example: --update, which can update references to latest dependency versions.
Actually I see a problem with this design: how can I pick the latest commit for ECS 1.9? Let me suggest something.
There was a problem hiding this comment.
Ok, I adjusted the manifest to include additional field "version". The update procedure would be as follows:
- Check ECS dependency considering version (e.g.
1.9). - Fetch latest commit from the branch
1.9. - Update
referencewith the commit hash.
Note:
I'm not fully satisfied with this approach (need to store version and source reference), but I don't see any other reasonable approach. We can always store an additional metafile somewhere around, but with this decision we could also introduce a special deps.yml instead of keeping everything in package manifest.
There was a problem hiding this comment.
@mtojek having a version and a reference can be confusing, even contradictory if someone sets a reference that doesn't correspond to a version. I think that we can go on by now with your original approach with the reference only, and we can improve it later to support other use cases.
how can I pick the latest commit for ECS 1.9?
Fetch latest commit from the branch
1.9.
I wouldn't try to support branches, at least by now, configured branches may lead to unreproducible builds, same code built at different moments can result in packages with different fields.
test/packages/good/manifest.yml
Outdated
| @@ -40,3 +40,8 @@ icons: | |||
| title: system | |||
| size: 1000x1000 | |||
| type: image/svg+xml | |||
| build: | |||
There was a problem hiding this comment.
If we put this into the "main" manifest.yml, it will also stay in the package when downloaded by Kibana. Do we want that? Or should it be really a build time definition that is removed? Maybe a separate file? Or similar / part of the test definitions?
There was a problem hiding this comment.
There are pros and cons of both options.
it will also stay in the package when downloaded by Kibana. Do we want that?
Pros:
- Kibana can link directly to ECS docs or repo to refer to the revisions
- We don't need to reverse engineer the ECS version used by the built package
Cons:
- Manifest contains new entries for dependencies
Or should it be really a build time definition that is removed?
I would be against modifying the manifest to get rid of dependencies. Maybe we can get rid of the build level and just leave dependencies.
Maybe a separate file?
Hm.. we can extract this logic to deps.yml. WDYT?
There was a problem hiding this comment.
- Agree on not modifying the manifest during build time
- Interesting point that Kibana could also use this information in the future to build reference links
Maybe deps.yml gives us best of both worlds? But I'm torn if adding more files is a good idea. One advantage of depds.yml is that there could be a command elastic-package deps update (just made it up) that automatically updates references in the deps.yml file. I would not necessarily do the same with the manifest.
There was a problem hiding this comment.
One advantage of depds.yml is that there could be a command elastic-package deps update (just made it up) that automatically updates references in the deps.yml file. I would not necessarily do the same with the manifest.
Yes, that's the plan. I was thinking about elastic-package build --update, which will try to pull latest commit references.
|
Ok, I extracted dependencies to an external file named "deps.yaml". |
test/packages/good/deps.yml
Outdated
| @@ -0,0 +1,4 @@ | |||
| build: | |||
There was a problem hiding this comment.
Is build redundant now here? What other dependencies can be in this file? If at some moment there are runtime dependencies they will need to be in the manifest, or somewhere downloaded by Kibana.
| build: |
Or perhaps this file can be called build.yml in case we need more things in the future only in build time. And this key could be called dependencies.
| build: | |
| dependencies: |
There was a problem hiding this comment.
I'm ok with build.yml too, although this may suggest to introduce more side features into it :)
We have also _dev directory, maybe this one is a better fit? For example: _dev/build/deps.yml. WDYT? Currently we're using the _dev/build/docs to render README files.
There was a problem hiding this comment.
I adjusted the PR to use build.yml in the package root directory.
| @@ -0,0 +1,3 @@ | |||
| dependencies: | |||
There was a problem hiding this comment.
Just approved the other PR but we have one more name here: dependency.
I assume the ecs dependency can only be used once, should we add it by name instead of array?
dependencies:
ecs:
reference: foo
There was a problem hiding this comment.
Replaced array with objects. I assume it's just to prevent custom dependencies that users may try to introduce and we don't want them now.
What does this PR do?
This PR introduces support for build-time dependencies.
Why is it important?
We need to checkout field definitions from the ECS repository and use them to compile up-to-date and synced fields.
Checklist
test/packagesthat prove my change is effective.versions/N/changelog.yml.Related issues