Bug 1842982: preserve ARG values that come before the first FROM #132

nalind · 2020-02-12T19:55:03Z

Some of the helper functions that addBuildParameters() calls modify the parsed tree, discarding ARG instructions that precede the first FROM instruction. The replacement Dockerfile that we generate from that tree would be missing those instructions.

Knowing that this will happen, walk the parsed tree beforehand, recording ARG instructions that precede the first FROM instruction, and then re-prepend them to the Dockerfile that we generate from our tree after we've finished manipulating it.

If we've got BuildArgs in a DockerStrategy, populate the BuildArgs map that we pass to imagebuilder functions that might have a use for them.

We usually try to pull images that are referenced in the Dockerfile ourselves, so that we can record the time required to pull them. If we looked for the image and encounter a problem other than image-hasn't-been-pulled (attempting to use an argument in the name would often lead to invalid-image-reference errors), we would previously have failed, but instead, we now let the build stage handle attempting to pull the image.

Some of the helper functions that addBuildParameters() calls modify the parsed tree, discarding ARG instructions that precede the first FROM instruction. The replacement Dockerfile that we generate from that tree would be missing those instructions. Knowing that this will happen, walk the parsed tree beforehand, recording ARG instructions that precede the first FROM instruction, and then re-prepend them to the Dockerfile that we generate from our tree after we've finished manipulating it. If we've got BuildArgs in a DockerStrategy, populate the BuildArgs map that we pass to imagebuilder functions that might have a use for them. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

We usually try to pull images that are referenced in the Dockerfile ourselves, so that we can record the time required to pull them. If we looked for the image and encounter a problem other than image-hasn't-been-pulled, we would previously have failed, but instead, we now let the build stage handle attempting to pull the image. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

gabemontero · 2020-02-13T15:12:23Z

pkg/build/builder/common.go

 	}

-	if err := replaceImagesFromSource(node, build.Spec.Source.Images); err != nil {
+	var buildArgs []corev1.EnvVar


Looks like you discovered a separate issue from the ARG one where docker strategy ENVs were getting lost as well @nalind

I'm I reading that correctly?

If so, would a comment, analogous to your ARG explanation above, placed here, and a tweak to the PR title/commit, seem prudent?

And some new unit tests as well, again assuming I'm reading this correctly.

They're still build args. The API struct is just called an EnvVar, I'm guessing because inventing a new type for every key-value pairing would have been excessive.

I'm not sure this added logic buys us anything, since we don't do any substitutions when we're walking the parsed tree, but I added it for completeness. I wouldn't object if we wanted to drop it.

Thanks for the clarification.

I don't feel either way about keeping it at the moment, though perhaps a unit test would prove one way or the other whether it buys us anything.

Unless other chime in with stronger opinions, I'll leave it at that.

nalind · 2020-02-13T15:28:09Z

/test e2e-aws-builds

danielkucera · 2020-05-29T11:13:25Z

Can we please get some attention here, this bug is affecting us, see: https://access.redhat.com/support/cases/#/case/02630699
if you have access.

gabemontero · 2020-05-29T12:29:51Z

is there a bug associated with that case @danielkucera ? ... it would need to be associated with this PR in order for it to merge

tests are green

and my review comment was addressed at least

/assign @adambkaplan

danielkucera · 2020-05-29T13:46:56Z

@gabemontero this was the original issue:
https://bugzilla.redhat.com/show_bug.cgi?id=1839683
but after latest update the error has changed to:

Caching blobs under "/var/cache/blobs".
error: build error: error parsing name "***/rhel8-ubi-base:${BASE_IMAGE_TAG}": error parsing image name "***/rhel8-ubi-base:${BASE_IMAGE_TAG}": invalid reference format

(registry was starred-out by me)

so now it depends on one of yours:
https://bugzilla.redhat.com/show_bug.cgi?id=1801875

gabemontero · 2020-05-29T13:54:10Z

@gabemontero this was the original issue:
https://bugzilla.redhat.com/show_bug.cgi?id=1839683
but after latest update the error has changed to:
Caching blobs under "/var/cache/blobs".
error: build error: error parsing name "***/rhel8-ubi-base:${BASE_IMAGE_TAG}": error parsing image name "***/rhel8-ubi-base:${BASE_IMAGE_TAG}": invalid reference format
(registry was starred-out by me)

so now it depends on one of yours:
https://bugzilla.redhat.com/show_bug.cgi?id=1801875

I see ... thanks for the info @danielkucera .... I think we need to let @nalind make the call on which BZ he thinks should be associated with this PR.

After that is done, we'll need an approve from @adambkaplan and then either he or I to lgtm this

adambkaplan · 2020-06-02T13:41:40Z

@nalind @gabemontero based on the discussion in this PR, I'm going to create a separate BZ to cover this specific case. There seem to be separate issues wrt args in Buildah that may need to be addressed.

adambkaplan

/approve

Going to create an appropriate BZ so we can try and get this into 4.5.0

adambkaplan · 2020-06-02T13:54:00Z

/retitle
Bug 1842982: preserve ARG values that come before the first FROM

adambkaplan · 2020-06-02T13:54:17Z

/retitle Bug 1842982: preserve ARG values that come before the first FROM

openshift-ci-robot · 2020-06-02T13:54:21Z

@nalind: This pull request references Bugzilla bug 1842982, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1842982: preserve ARG values that come before the first FROM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

adambkaplan

/lgtm

openshift-ci-robot · 2020-06-02T13:54:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, nalind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adambkaplan · 2020-06-02T13:54:54Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-06-02T13:54:54Z

@adambkaplan: once the present PR merges, I will cherry-pick it on top of release-4.5 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2020-06-03T07:28:33Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-06-03T09:02:12Z

@nalind: All pull requests linked via external trackers have merged: openshift/builder#132. Bugzilla bug 1842982 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1842982: preserve ARG values that come before the first FROM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2020-06-03T09:02:31Z

@adambkaplan: new pull request created: #156

Details

In response to this:

/cherry-pick release-4.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 12, 2020

openshift-ci-robot requested review from adambkaplan and coreydaley February 12, 2020 19:56

nalind force-pushed the preamble-args branch 2 times, most recently from a193302 to 480c819 Compare February 12, 2020 20:15

nalind added 2 commits February 12, 2020 16:00

nalind force-pushed the preamble-args branch from 480c819 to 6a5f838 Compare February 12, 2020 21:00

gabemontero reviewed Feb 13, 2020

View reviewed changes

danielkucera mentioned this pull request May 11, 2020

add test for instruction FROM with ARG instruction #153

Closed

openshift-ci-robot assigned adambkaplan May 29, 2020

adambkaplan approved these changes Jun 2, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2020

openshift-ci-robot changed the title ~~manage-dockerfile: preserve ARG values that come before the first FROM~~ Bug 1842982: preserve ARG values that come before the first FROM Jun 2, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jun 2, 2020

adambkaplan approved these changes Jun 2, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2020

openshift-merge-robot merged commit f69cc32 into openshift:master Jun 3, 2020

openshift-cherrypick-robot mentioned this pull request Jun 3, 2020

[release-4.5] Bug 1843405: preserve ARG values that come before the first FROM #156

Merged

nalind deleted the preamble-args branch June 16, 2020 19:20

Bug 1842982: preserve ARG values that come before the first FROM #132

Bug 1842982: preserve ARG values that come before the first FROM #132

Uh oh!

Conversation

nalind commented Feb 12, 2020

Uh oh!

gabemontero Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

gabemontero Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

nalind Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

gabemontero Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

nalind commented Feb 13, 2020

Uh oh!

danielkucera commented May 29, 2020

Uh oh!

gabemontero commented May 29, 2020

Uh oh!

danielkucera commented May 29, 2020

Uh oh!

gabemontero commented May 29, 2020

Uh oh!

adambkaplan commented Jun 2, 2020

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Jun 2, 2020

Uh oh!

adambkaplan commented Jun 2, 2020

Uh oh!

openshift-ci-robot commented Jun 2, 2020

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jun 2, 2020

Uh oh!

adambkaplan commented Jun 2, 2020

Uh oh!

openshift-cherrypick-robot commented Jun 2, 2020

Uh oh!

openshift-bot commented Jun 3, 2020

Uh oh!

openshift-ci-robot commented Jun 3, 2020

Uh oh!

openshift-cherrypick-robot commented Jun 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants