getporter / porter

Porter enables you to package your application artifact, client tools, configuration and deployment logic together as an installer that you can distribute, and install with a single command.
https://porter.sh
Apache License 2.0
1.23k stars 205 forks source link

Addition of experimental flag `full-control-dockerfile` #3114

Closed jarnfast closed 5 months ago

jarnfast commented 5 months ago

What does this change

Addition of experimental flag full-control-dockerfile

When used porter will use the file referenced by dockerfile without any changes (templating, CMD, et al)

Can be used for building truly custom invocation images. E.g.:

What issue does it fix

Closes #2802

Implemented as an experimental flag after discussion on Slack. The is a less intrusive solution than adding a CLI flag yet giving users the full control of the Dockerfile until a more thorough refactoring of the Dockerfile generation will be implemented.

Notes for the reviewer

Naming stuff is HARD - any suggestion on the experimental flag name will not be taken badly 😆

I opted to duplicate a bit of the "reading the Dockerfile" code in order to avoid injecting the IsFeatureEnabled conditional in multiple methods in the dockerfile-generator.go file. This also makes it easier to remove down the line.

Checklist

kichristensen commented 5 months ago

Hi, thank you very much for the PR.

A small nit, it might be a good idea when installing a bundle with the experimental feature enabled, to write a warning saying that changes to the dockerfile is not automatically detected. But it is not very important to me, as it is an experimental feature.

@schristoff how do we want to handle the updated documentation? Do we want to wait with merging of the documentation until around the release, or do we accept that the documentation is updated right away?

jarnfast commented 5 months ago

A small nit, it might be a good idea when installing a bundle with the experimental feature enabled, to write a warning saying that changes to the dockerfile is not automatically detected. But it is not very important to me, as it is an experimental feature.

Ah, actually this is the current behavior when using the dockerfile option. Ie. it's not introduced by this experimental feature. I'd gladly propose a separate improvement to make ensureLocalBundleIsUpToDate aware of the dockerfile contents 😄

kichristensen commented 5 months ago

@jarnfast that makes sense, let us keep it out of this PR

schristoff commented 5 months ago

@schristoff how do we want to handle the updated documentation? Do we want to wait with merging of the documentation until around the release, or do we accept that the documentation is updated right away?

Let's just merge it in, we are a month out so I'm not too concerned. In the future maybe we need to start a docs branch though 🤔

kichristensen commented 5 months ago

I'm going to approve because I'm only picking at documentation

I will second that. It is an advanced flag that most people won't use, unless they really know what they are doing.

jarnfast commented 5 months ago

I'm going to approve because I'm only picking at documentation

I will second that. It is an advanced flag that most people won't use, unless they really know what they are doing.

Cool, thanks, then I won't update the PR any further ;)