-
Notifications
You must be signed in to change notification settings - Fork 73
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.
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
Support auxiliary files in terraform deploy spec #272
Conversation
611ac47
to
0c75cd1
Compare
@mtojek I have a question about the changelog. The file header says
but it seems the opposite is true? Am I expected to append to |
dc3ecfd
to
04f26f8
Compare
Put it at the bottom of the 1.4.1 section, yes :) |
@@ -10,6 +10,9 @@ | |||
- description: Prepare for next version | |||
type: enhancement | |||
link: https://github.com/elastic/package-spec/pull/265 | |||
- description: Support ausiliary files in terraform deploy spec | |||
type: enhancement | |||
link: https://github.com/elastic/package-spec/pull/272 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I added a |
Test packages are used in |
4b38af4
to
dc0c152
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicking, but LGTM in general.
@@ -0,0 +1,3 @@ | |||
version: '2.3' | |||
services: | |||
hello_world: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. Is this file needed here? I guess it doesn't do any harm though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover from copying over the deploy_docker
module, thank you for catching this.
- FOO=${BAR:-default-value} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No but given the file is not required I'll remove it
versions/1/changelog.yml
Outdated
@@ -10,6 +10,9 @@ | |||
- description: Prepare for next version | |||
type: enhancement | |||
link: https://github.com/elastic/package-spec/pull/265 | |||
- description: Support ausiliary files in terraform deploy spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. Should it be "auxiliary"?
- description: Support ausiliary files in terraform deploy spec | |
- description: Support auxiliary files in terraform deploy spec |
pattern: '^*.json$' | ||
type: file | ||
contentMediaType: "application/json" | ||
required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we end-up needing additionalContents: false
if we find scenarios that would need arbitrary files. But we can reduce restrictions in the future if we find these cases.
versions/1/_dev/deploy/tf/spec.yml
Outdated
- description: Terraform dependency lock file | ||
pattern: ".terraform.lock.hcl" | ||
type: file | ||
required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, this may be breaking for current packages. At least we will need to add this file to the AWS package when updating the spec in the integrations repo.
https://github.com/elastic/integrations/tree/8e600daead4b89e97271e4976b984722c8db7c74/packages/aws/data_stream/ec2_metrics/_dev/deploy/tf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find, actually I don't want to introduce a breaking change here. Do we have a process here? I will make this not required, but I think it make sense to have it everywhere. Is there a way to notify users of this change? For example outputting a message: "this field is going to default to true from version x.y.z"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add this as optional by now, and after some time make it required. At the moment I think that only the AWS package uses it, so not many users affected :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's mark it optional for now.
I would have liked to set this file as required, but it would introduce a breaking change. So I added a note to update this requirement in the future.
dc0c152
to
9652403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What does this PR do?
Why is it important?
.tftpl
suggested extensionFurther discussion available in #269.
Checklist
test/packages
that prove my change is effective.versions/N/changelog.yml
.Related issues