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
Tweaks to the Iron Bank docker context #66942
Tweaks to the Iron Bank docker context #66942
Conversation
Pinging @elastic/es-delivery (Team:Delivery) |
@jmlrt Did you want the |
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.
One question, otherwise 👍
@@ -1,7 +1,7 @@ | |||
{ | |||
"resources": [ | |||
{ | |||
"url": "https://artifacts.elastic.co/downloads/elasticsearch/elasticsearch-${version}-linux-x86_64.tar.gz", | |||
"url": "<artifact_path>/elasticsearch-${version}-linux-x86_64.tar.gz", |
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.
What is this syntax? Does this value get replaced somewhere, and if so, how?
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.
It’s literally just a placeholder. Infra are planning a process to substitute the correct values during the unified release process.
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.
@jmlrt Did you want the validation section omitted entirely for the ES artifact in downloads.json?
Yes, the validation section should be omitted for ES artifacts.
@@ -364,6 +369,9 @@ LABEL name="Elasticsearch" \\ | |||
RUN mkdir /licenses && \\ | |||
cp LICENSE.txt /licenses/LICENSE | |||
<% } %> | |||
<% if (docker_base == 'iron_bank') { %> | |||
COPY LICENSE /licenses/LICENSE.addendum |
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.
https://github.com/elastic/ironbank/blob/master/Elastic_License_w_gov_addendum_2020-10-08-1.txt contains standard license + addendum so it should completely replace the other license file.
Can you also add this license file to the generated build context tarball?
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.
The simplest thing to do would be to copy that file into the ES repo and add it into the generated build context. What do you think? Obviously having it in two places isn't ideal, but fetching the license file when the task run also feels fragile.
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 totally agree 👍
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.
The license is now included in the repo and in the generated build context.
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⛴
This PR makes a few tweaks to the Docker context that the ES build can generate, in order to align it more closely with what we submit to DSOP.
This PR makes a few tweaks to the Docker context that the ES build can generate, in order to align it more closely with what we submit to DSOP.