feat[closes #38]: add workdir instruction support

[lambdaclan]

Add WORKDIR instruction support

Closes #38

⚠️NOTE⚠️

• I am marking this PR as a draft since a lot of things require feedback from the upstream developers.
• The proposed changes introduce breaking changes as they alter the recipe structure meaning that existing recipes will no longer build without adopting the new style.
• I will be happy to do any necessary adjustments to be in line with the upstream developers' vision of this feature.
• The workdir related checks for each supported command are the same so maybe we can extract it to avoid duplicated code. I want to avoid refactoring/simplification until we come to an agreement of how this feature should be implemented.

Description

This PR adds support for the WORKDIR instruction to various commands. It also checks to ensure that unnecessary WORKDIR instructions are ignored when the requested working directory is already the present working directory by having a recipe level PWD.

Proposed Changes

• Add WORKDIR instruction support for all possible commands:
  • RUN,
  • CMD
  • ENTRYPOINT
  • COPY
  • ADD
• Use a recipe wide PWD variable to keep track of the present working directory to avoid unnecessary (albeit harmless) instructions
• Restore working directory back to default path (/) after command execution
  -Always set the recipe's initial PWD to root (/) like default Docker
• Ensure that when using singlelayer: true WORKDIR mismatches do not occur (not sure if the check is needed so TBD)
• Add support for standard COPY (not only per stage copy via --from)
• Add support for multiple adds (like copy) each with different workdir if needed
• Update recipe structure documentation
• Fix doc formatting with prettier and mardownlint
• Fix formatting with gofumpt and goimportx

Local Testing

Using the following vib.yml recipe (based on the original chronos-fe recipe):

name: Chronos Frontend Image
id: chronos-fe
stages:
  - id: build
    base: node:lts
    singlelayer: false
    labels:
      maintainer: Vanilla OS Contributors
    adds:
      - workdir: /tmp/user/files
        srcdst:
          go.mod: .
      - workdir: /tmp/otheruser/files
        srcdst:
          go.mod: .
          go.sum: .
    copy:
      - workdir: /tmp
        from: build
        paths:
          - src: /app/awesome.txt
            dst: .
          - src: /tmp/test.txt
            dst: .
      - workdir: /etc
        paths:
          - src: /app/hello.txt
            dst: .
    cmd:
      workdir: /app2
      exec:
        - python
        - prog.py
    entrypoint:
      workdir: /app
      exec:
        - npm
        - run
        - build
    runs:
      workdir: /etc/apt/apt.conf.d
      commands:
        - echo 'APT::Install-Recommends "1";' > 01norecommends
    expose:
      "6090": ""
    modules:
      - name: build-app
        type: shell
        workdir: /app
        source:
          type: git
          url: https://github.com/Vanilla-OS/chronos-frontend
          branch: main
          commit: latest
        commands:
          - mv /sources/build-app .
          - npm install
          - npm run build
repo: vanilla-os/chronos-frontend

vib build vim.yml

Generated Containerfile:

# Stage: build
FROM node:lts AS build
WORKDIR /tmp
COPY --from=build /app/awesome.txt .
COPY --from=build /tmp/test.txt .
WORKDIR /
WORKDIR /etc
COPY /app/hello.txt .
WORKDIR /
LABEL maintainer='Vanilla OS Contributors'
WORKDIR /etc/apt/apt.conf.d
RUN echo 'APT::Install-Recommends "1";' > 01norecommends
WORKDIR /
EXPOSE 6090/
WORKDIR /tmp/user/files
ADD go.mod .
WORKDIR /
WORKDIR /tmp/otheruser/files
ADD go.mod .
ADD go.sum .
WORKDIR /
ADD includes.container /
ADD sources /sources
WORKDIR /app
RUN mv /sources/build-app . && npm install && npm run build
WORKDIR /
WORKDIR /app2
CMD ["python","prog.py"]
WORKDIR /
WORKDIR /app
ENTRYPOINT ["npm","run","build"]
WORKDIR /

[taukakao]

Thanks for contributing.

Please explain why you think that this feature requires breaking chages.

Also, I think that adding state here adds unecessary complexity. I would prefer a solution that just resets the working directory to a default value (probably "/") since changing the working directory is not a costly operation.

[lambdaclan]

Hello @taukakao.

Please explain why you think that this feature requires breaking chages.

I was trying to stay as close as possible to how an actual Dockerfile works. Each supported command can optionally specify its working directory before it executes. For each command to keep track of the working directory a new structure is needed.

Furthermore, without attaching workdir to a command state how we will know when to switch working directories? I mean YAML does not allow for duplicated keys so using workdir multiple times at the top level is not feasible. Originally I was thinking to only have a module/stage level workdir but then the workdir can only be changed once. I guess we could use other naming schemes maybe workdir+number to specify multiple workdirs at the top level but then we will need to keep track of what comes after that.

Also, I think that adding state here adds unecessary complexity. I would prefer a solution that just resets the working directory to a default value (probably "/") since changing the working directory is not a costly operation.

Indeed, I originally had a restore flag that would optionally restore the working directory back to root after the command is executed. I am not sure if it required, but we could re-introduce it. Regarding state itself, again as mentioned above it is needed to know for which command to switch the workdir for.

I am not sure how you intend to use this feature. For example, I am not sure if it is needed for all supported commands or if you want to set a working directory multiple times in a recipe.

This is just a draft implementation of how I pictured it working, and I was a bit worried about the breaking changes but of course I am open to suggestions/feedback and will be happy to adjust as necessary. We just need to keep in mind that keeping things a bit separate will allow us to further extend Vib's feature set down the line if needed. For example having separate states for each command will allow us to add more of their options like copy has from etc.

[mirkobrombin]

Looks good to me. I like the idea of having WORKDIR in all the available commands. Personally I do not understand where this is a breaking change. Can't we simply keep WORKDIR empty if not defined? From my understanding this would make existing recipes just works as before, correct me if I am wrong.

[lambdaclan]

Hello Mirko. Thank you very much for the feedback.

Personally I do not understand where this is a breaking change. Can't we simply keep WORKDIR empty if not defined? From my understanding this would make existing recipes just works as before, correct me if I am wrong.

Unfortunately, that is not the case.WORKDIR is indeed optional but the commands' structure has also changed. For example if we look at ENTRYPOINT previously it was expecting just a string array whereas now it is an EntryPoint struct:

This means that any recipe using entrypoint for example the chronos-fe recipe will need to be updated from:

entrypoint:
      - /bin/sh
      - '-c'
      - cd /app && npm run publish

to

entrypoint:
      exec:
      - /bin/sh
      - '-c'
      - cd /app && npm run publish

otherwise:

I am wondering if there is an easy way to avoid breaking changes by using generics. Ideally what we want for the entrypoint example is that it can either be a string[] or an EntryPoint type. This will complicate the state and the checks we need to do, but I guess it is possible. It depends on how far we want to go. I will see if I can find any other workarounds.

[mirkobrombin]

Oh, got it. I think it's fine, we already introduced breaking changes in the past, this just needs to be documented in our documentation.

[taukakao]

@lambdaclan

I find it very important that the individual commands stay modular, so I would prefer if:

    copy:
      - workdir: /tmp
        paths:
          - src: /app/awesome.txt
            dst: .

would create

WORKDIR /tmp
COPY /app/awesome.txt .
WORKDIR /

That would cause a lot of duplicate WORKDIR instructions but I don't think that's really a problem and it would remove the need for the state and guarantees that following instructions will not be affected by the workdir.

[taukakao]

Also, if it only breaks the entrypoint then I think it would be reasonable.
Are there other commands that need to be adjusted for this?

[lambdaclan]

Thank you everyone for the feedback.

@taukakao

Also, if it only breaks the entrypoint then I think it would be reasonable.
Are there other commands that need to be adjusted for this?

The new workdir instruction introduces a breaking change to all supported commands except copy. This is because copy was the only command using a custom object and could be easily extended to support additional options. This is what I mean that it might make sense to do the same for all commands to ensure we avoid reintroducing breaking changes going forwards.

RUN

before

runs:
  - echo "hello"

after:

runs:
  comands:
    - echo "hello"

CMD

before

cmd:
  - echo "hello"

after:

cmd:
  exec:
    - echo "hello"

ENTRYPOINT

before

entrypoint:
  - echo "hello"

after:

entrypoint:
  exec:
    - echo "hello"

ADD

before

adds:
  myfile:/tmp

after:

adds:
    srcdst:
      myfile:/tmp
    srcdst:
      myotherfile:/tmp

Now each of the commands above (including COPY) also takes an optional workdir to specify the working directory before each action is executed.

Can one of you please clarify the following points so that I can proceed:

• Is it OK to introduce the breaking changes above?
• Is it OK to make each workdir command automatically revert to root as requested by @taukakao?
• Is it OK to remove recipe wide PWD state? This means that we will no longer check if the workdir instruction changes to the same working directory - also depends on above point.

  runs:
    workdir: /tmp
    commands:
      - echo "hello"
  cmd:
    workdir: /tmp
    exec:
      - echo "world"

will generate

WORKDIR /tmp
RUN echo "hello"
# WORKDIR / -> if we always revent back to root
WORKDIR /tmp
CMD [echo "world"]
# WORKDIR / -> if we always revent back to root

• If we are going to introduce breaking changes would you like to also update copy to behave like add? I find the current setup inconsistent because add takes a map for source/destination whereas copy take a different path object. They should both take a map or a path object.
  

• How should we handle workdir when singlelayer = true? Right now it seems all RUN commands get joined with commands from the modules. This can be problematic because the run command and module commands might specify a different workdir causing mismatches. Currently, if singlelayer=true I compare workdirs (if specified) and throw an error if they do not match.

Let me know when you get a chance.

Thank you!

[taukakao]

Is it OK to introduce the breaking changes above?

Yeah, it's unfortunate but I see why it's necessary. At least it will make it easier in the future.

@mirkobrombin for the other questions

If we are going to introduce breaking changes would you like to also update copy to behave like add?

I think that would be sensible, but it should be done in two different commits or maybe even two different PRs.

Currently, if singlelayer=true I compare workdirs (if specified) and throw an error if they do not match.

I think that's a reasonable approach.

[mirkobrombin]

Is it OK to make each workdir command automatically revert to root as requested by @taukakao?

it is fine

Is it OK to remove recipe wide PWD state? This means that we will no longer check if the workdir instruction changes to the same working directory - also depends on above point.

it is fine if necessary

[lambdaclan]

Updated the PR with the requested changes.
I will be doing some more local testing while I wait for your review.
Regarding the copy/add structure mismatch I will be creating a different PR later on.

Thank you!

Edit: Example and generated container file in the original description have also been updated for your reference.

[mirkobrombin]

Tested with the Containerfile in the first comment. Looks good to me, nice job!
Waiting for at least another reviewer.

[taukakao]

Just one tiny note. Otherwise it looks really good.

Also tested building the desktop image. Just had to rebuild fsguard with the new api and change one line in the recipe and it worked.

[lambdaclan]

Latest commits:

• Restore comment removal (auto formatting issue)
• Update docs
• Update example and generated containerfile

Please note that CMD and ENTRYPOINT uses the exec form ["", ""] so we need to pass each command as a list which is why I used exec as a field name

entrypoint:
  exec:
    - npm run build

["npm run build"] --> wrong

entrypoint:
  exec:
    - npm
    - run
    -  build

["npm", "run", "build"] --> correct

[taukakao]

LGTM

[kbdharun]

LGTM, Thanks for your contribution. Tested the changes and checked the docs additions.

[taukakao]

Mirko already approved it, so I'm gonna merge it.

[taukakao]

Thanks for your contribution @lambdaclan

[taukakao]

(And yes, I just realized that those commits weren't squashed yet, we will decide how to deal with that)