Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define build time dependencies #181

Merged
merged 10 commits into from
Jun 14, 2021
Merged

Define build time dependencies #181

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c93c184
Define build time dependencies
Jun 2, 2021
97435a3
Update changelog
Jun 2, 2021
a8915b1
Address PR comments
mtojek Jun 7, 2021
10552d7
Merge branch 'master' into 179-build-dependencies
Jun 9, 2021
ef12409
Extract to deps.yaml
Jun 9, 2021
c5c33aa
Rename deps.yml to build.yml
Jun 10, 2021
7ccc2aa
Move build.yml under _dev/build
Jun 10, 2021
57e9fba
Merge branch 'master' into 179-build-dependencies
Jun 10, 2021
b532033
Replace array with obj
Jun 10, 2021
ec81ed5
Fix
Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion code/go/internal/spec/statik.go
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions test/packages/good/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,8 @@ icons:
title: system
size: 1000x1000
type: image/svg+xml
build:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mtojek mtojek Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dependencies:
- name: ecs
version: "1.9"
reference: "git@7a11f3e1f7d8f05e8f06623fd98254b8cb34c441"
5 changes: 4 additions & 1 deletion versions/1/changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,7 @@
link: https://github.com/elastic/package-spec/pull/175
- description: Define common test config for pipeline tests
type: enhancement
link: https://github.com/elastic/package-spec/pull/176
link: https://github.com/elastic/package-spec/pull/176
- description: Define build-time dependencies
type: enhancement
link: https://github.com/elastic/package-spec/pull/181
28 changes: 28 additions & 0 deletions versions/1/manifest.spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,34 @@ spec:
pattern: '^(([a-zA-Z0-9-]+)|([a-zA-Z0-9-]+\/[a-zA-Z0-9-]+))$'
required:
- github
build:
type: object
additionalProperties: false
properties:
dependencies:
type: array
description: Build-time dependencies
items:
type: object
additionalProperties: false
properties:
name:
type: string
description: Name of the dependency
enum:
- ecs
version:
type: string
description: Version of the dependency
reference:
type: string
description: Source reference
pattern: 'git@[a-z0-9]{40}'
required:
- name
- reference
required:
- dependencies
required:
- format_version
- name
Expand Down