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

FIX: missing or partial support for pattern substition in variable when cache enabled #2968

Merged

Conversation

kt315
Copy link
Contributor

@kt315 kt315 commented Jan 25, 2024

Fixes #1246

Description

There is no need to interpolate the variables in the RUN command to calculate the hash because the ARG and ENV variables are added when the CompositeCache is built.
Therefore, we can now use all substitutions from bash.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

@kt315
Copy link
Contributor Author

kt315 commented Jan 25, 2024

Dockerfile:

FROM ubuntu:20.04

ARG APP_VERSION=blabla
RUN echo ${APP_VERSION%-*} > test

build

### first time
test_kaniko$ sudo docker run -t --rm -v "$(pwd):/workspace/" gcr.io/kaniko-project/executor --insecure --insecure-pull --use-new-run --cache --cache-run-layers --cache-copy-layers --destination       "10.22.0.52:5000/blabla:test" --build-arg APP_VERSION=blabla2
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Retrieving image ubuntu:20.04 from registry index.docker.io
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Built cross stage deps: map[]
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Executing 0 build triggers
INFO[0000] Building stage 'ubuntu:20.04' [idx: '0', base-idx: '-1']
INFO[0000] Checking for cached layer 10.22.0.52:5000/blabla/cache:8e47b29493a39249c8315644e0c0a393ef91bca5c116643d9dda71268b9c63b1...
INFO[0000] No cached layer found for cmd RUN echo ${APP_VERSION%-*} > test
INFO[0000] Unpacking rootfs as cmd RUN echo ${APP_VERSION%-*} > test requires it.
INFO[0001] ARG APP_VERSION=blabla
INFO[0001] No files changed in this command, skipping snapshotting.
INFO[0001] RUN echo ${APP_VERSION%-*} > test
INFO[0001] Cmd: /bin/sh
INFO[0001] Args: [-c echo ${APP_VERSION%-*} > test]
INFO[0001] Running: [/bin/sh -c echo ${APP_VERSION%-*} > test]
INFO[0001] Taking snapshot of files...
INFO[0001] Pushing layer 10.22.0.52:5000/blabla/cache:8e47b29493a39249c8315644e0c0a393ef91bca5c116643d9dda71268b9c63b1 to cache now
INFO[0001] Pushing image to 10.22.0.52:5000/blabla/cache:8e47b29493a39249c8315644e0c0a393ef91bca5c116643d9dda71268b9c63b1
INFO[0006] Pushed 10.22.0.52:5000/blabla/cache@sha256:1792487d47569fdb19eaaa84227755e2764bee2c86822f2458bb8c34f0fd1a40
INFO[0006] Pushing image to 10.22.0.52:5000/blabla:test
INFO[0011] Pushed 10.22.0.52:5000/blabla@sha256:41df57b887206148d1361f9bc5ade6659cfbb1677b111977d25ebd34230c30c9

### second time
test_kaniko$ sudo docker run -t --rm -v "$(pwd):/workspace/" gcr.io/kaniko-project/executor --insecure --insecure-pull --use-new-run --cache --cache-run-layers --cache-copy-layers --destination       "10.22.0.52:5000/blabla:test" --build-arg APP_VERSION=blabla2
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Retrieving image ubuntu:20.04 from registry index.docker.io
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Built cross stage deps: map[]
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Executing 0 build triggers
INFO[0000] Building stage 'ubuntu:20.04' [idx: '0', base-idx: '-1']
INFO[0000] Checking for cached layer 10.22.0.52:5000/blabla/cache:8e47b29493a39249c8315644e0c0a393ef91bca5c116643d9dda71268b9c63b1...
INFO[0002] Using caching version of cmd: RUN echo ${APP_VERSION%-*} > test
INFO[0002] Skipping unpacking as no commands require it.
INFO[0002] ARG APP_VERSION=blabla
INFO[0002] No files changed in this command, skipping snapshotting.
INFO[0002] RUN echo ${APP_VERSION%-*} > test
INFO[0002] Found cached layer, extracting to filesystem
INFO[0005] Pushing image to 10.22.0.52:5000/blabla:test
INFO[0010] Pushed 10.22.0.52:5000/blabla@sha256:cfc8daa2946f8cc105d0305eb12c7825408db8b8dbedb328065c8fecfc9a920e

without fix

test_kaniko$ sudo docker run -t --rm -v "$(pwd):/workspace/" registry/build_system/kaniko --insecure --insecure-pull --use-new-run --cache --cache-run-layers --cache-copy-layers --destination       "10.22.0.52:5000/blabla:test"
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Retrieving image ubuntu:20.04 from registry index.docker.io
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Built cross stage deps: map[]
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Retrieving image manifest ubuntu:20.04
INFO[0000] Returning cached image manifest
INFO[0000] Executing 0 build triggers
INFO[0000] Building stage 'ubuntu:20.04' [idx: '0', base-idx: '-1']
error building image: error building stage: failed to optimize instructions: failed to process "RUN echo ${APP_VERSION%-*} > test": missing ':' in substitution

@kt315
Copy link
Contributor Author

kt315 commented Jan 26, 2024

@aaron-prindle, hi! Review pls)

Copy link
Collaborator

@QuanZhang-William QuanZhang-William left a comment

Choose a reason for hiding this comment

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

Changes look good to me. @aaron-prindle, please take a look if you are interested, otherwise we can merge it today

@kt315 kt315 force-pushed the 1246_fix_substition_in_variable branch from 23d2d4b to d206fb9 Compare January 27, 2024 09:10
Copy link
Collaborator

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kt315, LGTM!

@aaron-prindle aaron-prindle merged commit da3878e into GoogleContainerTools:main Feb 14, 2024
10 checks passed
@kt315 kt315 deleted the 1246_fix_substition_in_variable branch March 27, 2024 09:32
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.

Missing or partial support for pattern substition in variable references (e.g. in RUN)
3 participants