fix(ci): convert ci is failing due to perms

[rchincha]

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

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

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:

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

[rchincha]

https://github.com/project-stacker/stacker/actions/runs/4360350946/jobs/7625179702

^ pls see.

[codecov]

Codecov Report

Merging #439 (dc05c16) into main (8e267fc) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head dc05c16 differs from pull request most recent head 69f6e16. Consider uploading reports for the commit 69f6e16 to get more accurate results

@@            Coverage Diff             @@
##             main     #439      +/-   ##
==========================================
- Coverage   11.87%   11.86%   -0.01%     
==========================================
  Files          45       45              
  Lines        5551     5554       +3     
==========================================
  Hits          659      659              
- Misses       4775     4778       +3     
  Partials      117      117              

Impacted Files	Coverage Δ
pkg/stacker/convert.go	0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[smoser]

If I understand this correctly this comes from tests/convert.bats which has VOLUME /out and then 'mkdir -p /out'.
Would it be an option to just change that to go to a different directory?

    cat > Dockerfile <<EOF
FROM alpine:3.16
VOLUME $HOME/out
...

It just seems odd to attempt that as an unprivileged user is not likely to be able to create and use a directory in /

[rchincha]

If I understand this correctly this comes from tests/convert.bats which has VOLUME /out and then 'mkdir -p /out'. Would it be an option to just change that to go to a different directory?

    cat > Dockerfile <<EOF
FROM alpine:3.16
VOLUME $HOME/out
...

It just seems odd to attempt that as an unprivileged user is not likely to be able to create and use a directory in /

We could, but stacker convert would have no way to know what docker -v <outside-container-volume>:<inside-mount> the outside-container-volume would be. This is just so that we show we can be fully automated. But one is of course welcome to mangle the output of stacker convert and then mangle.

[smoser]

If I understand this correctly this comes from tests/convert.bats which has VOLUME /out and then 'mkdir -p /out'. Would it be an option to just change that to go to a different directory?

    cat > Dockerfile <<EOF
FROM alpine:3.16
VOLUME $HOME/out
...

It just seems odd to attempt that as an unprivileged user is not likely to be able to create and use a directory in /

We could, but stacker convert would have no way to know what docker -v <outside-container-volume>:<inside-mount> the outside-container-volume would be. This is just so that we show we can be fully automated. But one is of course welcome to mangle the output of stacker convert and then mangle.

just give an error:

Docker file 'Dockerfile'  not supported.  The provided Dockerfile contains a VOLUME entry for '/out'.
/out is not writable by the current user.  Please modify the Dockerfile to contain a writable path for
the given VOLUME.

[smoser]

I think this is really good. Thanks.

The only comment i have is that I would suggest processing command line arguments in order rather than always giving precedence to '--substitute' over contents of '--substitute-file'.

That allows the caller to get whatever behavior they want (perhaps they just want to append a '--substitute-file' onto an existing command line with --substitute and have it override).

its not a big thing, due to the "static" nature of the substitute-file, so i'll approve but you're welcome to change that too.