Skip to content
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

feat: copy add alignment #63

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

lambdaclan
Copy link
Contributor

Adjust COPY instruction to be in line with ADD instruction

⚠️NOTE⚠️

  • 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.

Description

This PR adjusts the COPY command structure to be in line with the similar ADD command. This ensures consistency and closely emulates how raw Dockerfiles work.

Proposed Changes

  • Update Copy structure to use a map to specify source and destination
  • Remove the unecessary Path structure
  • Update recipe structure documentation

Example

Before

copy:
      - workdir: /tmp
        from: build
        paths:
          - src: /app/awesome.txt
            dst: .
          - src: /tmp/test.txt
            dst: .
      - workdir: /etc
        paths:
          - src: /app/hello.txt
            dst: .

After

copy:
      - workdir: /tmp
        from: build
        srcdst:
          /app/awesome.txt: .
          /tmp/test.txt: .
      - workdir: /etc
        srcdst:
            /app/hello.txt: .

Local Testing

Using the following vib.yml recipe:

name: Copy Add Example
id: copy-add-example
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
        srcdst:
          /app/awesome.txt: .
          /tmp/test.txt: .
      - workdir: /etc
        srcdst:
          /app/hello.txt: .
vib build vim.yml

example

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 /tmp/user/files
ADD go.mod .
WORKDIR /
WORKDIR /tmp/otheruser/files
ADD go.mod .
ADD go.sum .
WORKDIR /
ADD includes.container /
ADD sources /sources

@lambdaclan
Copy link
Contributor Author

I feel we need to bump the version of Vib to reflect the new overall structure including the previously merged workdir changes. Please let me know what you think.

Thank you!

@kbdharun kbdharun requested review from mirkobrombin and kbdharun July 5, 2024 05:51
This was referenced Jul 5, 2024
@taukakao
Copy link
Member

taukakao commented Jul 5, 2024

While I preferred the syntax of copy over that of add I think it's too late to change this now either way.
So this change is ok with me.

And yes, I agree that this should be a new vib version.

@lambdaclan
Copy link
Contributor Author

lambdaclan commented Jul 6, 2024

Hello Tau.

While I preferred the syntax of copy over that of add I think it's too late to change this now either way.
So this change is ok with me.

I went with the ADD syntax because it is simpler (less typing) and behaves more like an actual Dockerfile.
Saying that, I do not want to impose anything so if the team prefers the original COPY syntax please let me know. I will be happy to do the necessary changes.

What really matters in the end is consistency.

@taukakao
Copy link
Member

taukakao commented Jul 6, 2024

if the team prefers the original COPY syntax please let me know

It's just my personal opinion. I guess we can wait if other people have an opinion as well.
But as I said, I think this is fine.

@lambdaclan lambdaclan force-pushed the feat/copy-add-alignment branch from a702cf9 to 5eba5fa Compare July 8, 2024 01:08
Copy link
Member

@mirkobrombin mirkobrombin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kbdharun kbdharun self-assigned this Jul 8, 2024
@lambdaclan lambdaclan force-pushed the feat/copy-add-alignment branch from 5eba5fa to 69e044f Compare July 9, 2024 00:41
Copy link
Member

@taukakao taukakao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@kbdharun kbdharun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks for your contribution. Tested and verified the changes locally.

@kbdharun kbdharun merged commit e11b8e8 into Vanilla-OS:main Jul 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants