add: report error while creating dir for URL source. #6087

flouthoc · 2025-03-25T18:18:33Z

Correctly report back error when attempting to create Tmpdir for a
given url source.

Also remove superfluous isGitTag from define/types.go since git fetch correctly resolves by provided reference.

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

add: correctly resolve git tags and branches and report errors if no reference found

nalind · 2025-03-25T18:20:31Z

add.go

@@ -517,7 +517,7 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 			if sourceIsGit(src) {
 				go func() {
 					var cloneDir, subdir string
-					cloneDir, subdir, getErr = define.TempDirForURL(tmpdir.GetTempDir(), "", src)
+					cloneDir, subdir, dirErr = define.TempDirForURL(tmpdir.GetTempDir(), "", src)


This should just return if getErr is not nil, as there's no point in continuing, and cloneDir and subdir are not expected to be useful values.

flouthoc · 2025-03-25T19:21:09Z

define/types.go

@@ -260,13 +260,6 @@ func parseGitBuildContext(url string) (string, string, string) {
 	return gitBranchPart[0], gitSubdir, gitBranch
 }

-func isGitTag(remote, ref string) bool {


There is no need to for this because fetch below automatically fails if reference does not exists.

flouthoc · 2025-03-25T19:21:47Z

define/types.go

@@ -284,12 +277,6 @@ func cloneToDirectory(url, dir string) ([]byte, string, error) {
 		return combinedOutput, gitSubdir, fmt.Errorf("failed while performing `git remote add`: %w", err)
 	}

-	if gitRef != "" {


There is no need for this fetch automatically resolves correct tag, branch or commit. If not found it fails.

add.go

nalind · 2025-03-25T19:28:38Z

tests/bud.bats

+
+  cat > $contextdir/Dockerfile << _EOF
+FROM alpine
+RUN apk add git


The image shouldn't need git installed for ADD to work, so this step is unnecessary. It can be FROM scratch if it doesn't need to run anything after the content has been added.

nalind

Changes look good, but the commit log doesn't exactly reflect what was changed.

flouthoc · 2025-03-26T01:10:56Z

@containers/buildah-maintainers PTAL

Luap99 · 2025-03-26T11:13:05Z

add.go

+					defer wg.Done()
+					defer pipeWriter.Close()


The defer placement feels wrong, doing this after the function call but before errors checking is odd.
IMO they should be at the top of the function.

Luap99 · 2025-03-26T11:14:57Z

tests/bud.bats

+
+  cat > $contextdir/Dockerfile << _EOF
+FROM scratch
+ADD https://github.com/openshift/microshift.git#foobar /src


We have no direct control over this repo, if they ever add such a branch all our CI would break.
Can we use buildah instead?

Honny1

Other than @nalind's comments, LGTM

Luap99 · 2025-03-26T15:07:29Z

tests/bud.bats

+ADD https://github.com/containers/crun.git#nosuchbranch /src
+_EOF
+  run_buildah 128 build -f $contextdir/Dockerfile -t git-image $contextdir
+  expect_output --substring "couldn't find remote ref foobar"


you switched to nosuchbranch but the test matches still foobar

Correctly report back error when attempting to create `Tmpdir` for a given url source. Also remove superfluous `isGitTag` from define/types.go since `git fetch` correctly resolves by provided reference. Closes: containers/podman#25679 Signed-off-by: flouthoc <[email protected]>

Luap99

/lgtm

But I don't have merge powers here I belive

openshift-ci · 2025-03-26T17:55:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99, nalind

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nalind]

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

flouthoc mentioned this pull request Mar 25, 2025

Silently ignore the branch section as part of url delimeter for ADD <git_url> containers/podman#25679

Closed

nalind reviewed Mar 25, 2025

View reviewed changes

flouthoc force-pushed the add-report-err branch 2 times, most recently from 0866948 to 752b5be Compare March 25, 2025 19:20

flouthoc commented Mar 25, 2025

View reviewed changes

flouthoc requested a review from nalind March 25, 2025 19:22

nalind requested changes Mar 25, 2025

View reviewed changes

flouthoc force-pushed the add-report-err branch from 752b5be to d6d384f Compare March 25, 2025 20:01

flouthoc requested a review from nalind March 25, 2025 20:01

nalind approved these changes Mar 25, 2025

View reviewed changes

openshift-ci bot added the approved label Mar 25, 2025

flouthoc force-pushed the add-report-err branch from d6d384f to 88c4e2d Compare March 25, 2025 20:42

flouthoc changed the title ~~add: correctly resolve git tags and branches~~ add: report error while creating dir for URL source. Mar 25, 2025

Luap99 reviewed Mar 26, 2025

View reviewed changes

Honny1 reviewed Mar 26, 2025

View reviewed changes

flouthoc force-pushed the add-report-err branch 2 times, most recently from 6dd6a43 to 8944852 Compare March 26, 2025 15:05

flouthoc requested a review from Luap99 March 26, 2025 15:05

Luap99 reviewed Mar 26, 2025

View reviewed changes

flouthoc force-pushed the add-report-err branch from 8944852 to 0c37781 Compare March 26, 2025 15:11

flouthoc requested a review from Luap99 March 26, 2025 15:18

Luap99 approved these changes Mar 26, 2025

View reviewed changes

openshift-ci bot assigned Luap99 Mar 26, 2025

openshift-ci bot added the lgtm label Mar 26, 2025

openshift-merge-bot bot merged commit e4d3dc0 into containers:main Mar 26, 2025
35 of 37 checks passed

flouthoc mentioned this pull request Mar 27, 2025

Fix windows path handling in podman cp containers/podman#25701

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: report error while creating dir for URL source. #6087

add: report error while creating dir for URL source. #6087

flouthoc commented Mar 25, 2025 •

edited

Loading

nalind Mar 25, 2025

flouthoc Mar 25, 2025

flouthoc Mar 25, 2025

nalind Mar 25, 2025

nalind left a comment

flouthoc commented Mar 26, 2025

Luap99 Mar 26, 2025

Luap99 Mar 26, 2025

Honny1 left a comment •

edited

Loading

Luap99 Mar 26, 2025

flouthoc Mar 26, 2025

Luap99 left a comment

openshift-ci bot commented Mar 26, 2025

add: report error while creating dir for URL source. #6087

add: report error while creating dir for URL source. #6087

Conversation

flouthoc commented Mar 25, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

flouthoc commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Honny1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 26, 2025

flouthoc commented Mar 25, 2025 •

edited

Loading

Honny1 left a comment •

edited

Loading