fix: stacker convert should include ENV vars into the runtime config.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.

[jmblatten]

Dockerfile ENV defines variables for both build and runtime. Stacker convert, converts ENV for build, but does not include runtime. Stacker uses "environment:" in yaml to define variables for runtime. So, include the ENV variables there so they will show up in the config when running the container.
See https://docs.docker.com/reference/dockerfile/#env

What type of PR is this?
bug

Which issue does this PR fix:
#757

What does this PR do / Why do we need it:
See above.

If an issue # is not available please add repro steps and logs showing the issue:
#757

Steps:

Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml

mybase:
build_env:
arch: amd64
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

Note: env does not include HELLO and GOODBYE in image.

Testing done on this change:

• CI
• Manual:
  Dockerfile contents:

FROM alpine AS mybase
ENV HELLO=hello!
ENV GOODBYE=goodbye!
RUN echo $HELLO && \
echo $GOODBYE

Converted stacker.yaml contents

mybase:
build_env:
arch: amd64
environment:
GOODBYE: goodbye!
HELLO: hello!
from:
type: docker
url: docker://alpine
run:
- export HELLO=hello!
- export GOODBYE=goodbye!
- sh -e -c 'echo $HELLO && echo $GOODBYE'

Using skopeo inspect,

"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"GOODBYE=goodbye!",
"HELLO=hello!"
]

Automation added to e2e:
None

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
Yes, but only for converted Dockerfiles. The resulting stacker.yaml file will now include an environment section if there were any ENVs in the Dockerfile. This will cause the image runtime to include the environment variables.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[hallyn]

Lgtm, thanks

[raharper]

I don't quite follow two things.

1. if this test is named 'invalid env' ; what it is testing?
2. Why is this hunk needed vs the original relating to the "invalid env" scenario?

I apologize here, I should study the test case here; but if you already know I'd appreciate some background on what this change is doing.

Lastly; I was hoping to see a test that demonstrates the failure so it's easy to reason about the fix.

[jmblatten]

Hi!
First, this testcase tests that "ENV FOO" will error out since it is syntactically incorrect. convertCommand() checks len(cmd.Value) < 3 for env and should error out.
It should be something like "ENV FOO=something". This patch does not change that behaviour.

This patch permits including the env vars defined in ENV(s) to be included into the runtime config for the image.

convertCommand() has a case stmt that handles conversions. A FROM conversion defines (declares and initializes) a layer and sets c.currLayer and c.output[c.currLayer]. Thus permitting subsequent convertCommand() calls to use that layer definition to add to runtime config if needed.
(i.e. ENV conversion sets layer.Environment)

So the invalid testcases sends data thru the convertCommand(). Before this patch, that particular testcase only needed a converter struct (NewConverter) to put the env vars into c.env for inclusion into the resulting stacker.yaml file. Now, with this patch, it also needs to add to runtime config using layer.Environment.
But, there isn't a FROM in the testcase data to set a layer definition.

To prevent a segfault, the invalid testcases whose conversions in convertCommand() use a layer definition for runtime config, set c.currLayer and c.output[c.currLayer] in the testcase's converter struct. Thus those first 3 lines in convertCommand() will result in setting a layer definition for the conversions to use.
Thus that extra chunk of code for that testcase. Otherwise, the testcase's behaviour should not change.