Repository: GoogleContainerTools/skaffold
Pull request #7251 (2022-03-31 08:58:45)

fix: fix bazel-out path for `rules_docker-0.23.0`
<!-- Thank you for your contribution! -->

<!-- Include if applicable: -->
Fixes: #7228 <!-- tracking issues that this PR will close -->
**Related**: https://github.com/bazelbuild/rules_docker/pull/1963
**Merge before/after**: _Dependent or prerequisite PRs_

**Description**
<!-- Describe your changes here. The more detail, the easier the review! -->
With the release of https://github.com/bazelbuild/rules_docker/releases/tag/v0.23.0, the output directory for bazel targets is calculated differently and is no longer a static path.

For example, a `container_image` target such as:
```
//foo/bar/server:server_img.tar
```

Previously was stored at:
```
bazel-out/k8-fastbuild/bin/foo/bar/server/server_img.tar
```

Internally, `skaffold` calculated this path by running `bazel info bazel-bin`, which returns the start of the path (e.g. `/home/...../execroot/__main__/bazel-out/k8-fastbuild/bin`), then appended the name of the rule with `:` replaced with `/` (e.g. `foo/bar/server/server_img.tar`). This results in the correct path as above.

However, with `rules_docker-0.23.0`, the output is now stored at a path like:
```
bazel-out/k8-fastbuild-ST-4a519fd6d3e4/bin/foo/bar/server/server_img.tar
```

So `bazel info` no longer returns the correct path prefix.

Using `bazel cquery` with a similar method to that described in https://github.com/bazelbuild/rules_pkg/tree/main/examples/where_is_my_output, as recommended in https://github.com/bazelbuild/rules_docker/pull/1963#issuecomment-1081197607, we can query Bazel for the actual output path to the target. Docker `.tar` output files are available in `cquery` as targets with a `.files` attribute, which contains the list of output files (there's only one), and each file has a `.path` attribute that gives the path to the file, which we can use internally in Skaffold. This is also backwards compatible with older versions of https://github.com/bazelbuild/rules_docker.

For example, the command:
```
bazel cquery //foo/bar/server:server_img.tar --output starlark --starlark:expr="target.files.to_list()[0].path"
```
May return:
```
bazel-out/k8-fastbuild/bin/foo/bar/server/server_img.tar
``` 
And with `rules_docker-0.23.0`, this may return something like:
```
bazel-out/k8-fastbuild-ST-4a519fd6d3e4/bin/foo/bar/server/server_img.tar
```
Which is the correct path to the target.

<!--
Please be sure your PR includes unit tests - we don't merge code that brings down test coverage! 
Integration tests are sometimes an appropriate substitute. 
-->


MarlonGamez: hi @sashamor , Thanks for opening this change! It looks like you'll have to go in and update `TestBuildBazel` unit test as well, but other than that these changes look good :)


sashamor: Thanks, fixed :) Also changed the arguments to no longer use nested quotes.

Tested locally with https://github.com/bazelbuild/rules_docker v0.23.0 and v0.22.0 and can confirm they both find the output tar correctly as expected.
Anthony-Bible: I wanted to add another data point, I had a similar problem and it worked when running with your patch.
codecov[bot]: # [Codecov](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) Report
> Merging [#7251](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) (3fd50d1) into [main](https://codecov.io/gh/GoogleContainerTools/skaffold/commit/290280e8c6c19231e9ed5b5c027f2c01d22f75be?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) (290280e) will **decrease** coverage by `2.02%`.
> The diff coverage is `56.96%`.

```diff
@@            Coverage Diff             @@
##             main    #7251      +/-   ##
==========================================
- Coverage   70.48%   68.45%   -2.03%     
==========================================
  Files         515      560      +45     
  Lines       23150    26507    +3357     
==========================================
+ Hits        16317    18146    +1829     
- Misses       5776     7106    +1330     
- Partials     1057     1255     +198     
```


| [Impacted Files](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) | Coverage Δ | |
|---|---|---|
| [cmd/skaffold/app/cmd/deploy.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZGVwbG95Lmdv) | `52.00% <ø> (-1.85%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/dev.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZGV2Lmdv) | `84.61% <0.00%> (ø)` | |
| [cmd/skaffold/app/cmd/render.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvcmVuZGVyLmdv) | `36.66% <0.00%> (-4.72%)` | :arrow_down: |
| [cmd/skaffold/skaffold.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL3NrYWZmb2xkLmdv) | `0.00% <0.00%> (ø)` | |
| [cmd/skaffold/app/cmd/inspect\_tests.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvaW5zcGVjdF90ZXN0cy5nbw==) | `62.50% <14.28%> (-1.14%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/lsp.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvbHNwLmdv) | `28.12% <28.12%> (ø)` | |
| [cmd/skaffold/app/cmd/fix.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZml4Lmdv) | `68.85% <40.00%> (-7.62%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/lint.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvbGludC5nbw==) | `42.85% <42.85%> (ø)` | |
| [cmd/skaffold/app/cmd/find\_configs.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZmluZF9jb25maWdzLmdv) | `48.88% <50.00%> (+0.24%)` | :arrow_up: |
| [cmd/skaffold/app/skaffold.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9za2FmZm9sZC5nbw==) | `76.19% <70.00%> (-8.43%)` | :arrow_down: |
| ... and [221 more](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7251/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) | |

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools)

MarlonGamez: @sashamor , thanks for making the change! I have a fix open for the github action failure we're seeing 
https://github.com/GoogleContainerTools/skaffold/pull/7263
Once this is merged, could you rebase this PR to retrigger the tests? I think it should be fixed then  

Patch:
diff --git a/pkg/skaffold/build/bazel/build.go b/pkg/skaffold/build/bazel/build.go
index 92cf14a80..12eceb865 100644
--- a/pkg/skaffold/build/bazel/build.go
+++ b/pkg/skaffold/build/bazel/build.go
@@ -23,7 +23,6 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
@@ -80,12 +79,11 @@ func (b *Builder) buildTar(ctx context.Context, out io.Writer, workspace string,
 		return "", fmt.Errorf("running command: %w", err)
 	}
 
-	bazelBin, err := bazelBin(ctx, workspace, a)
+	tarPath, err := bazelTarPath(ctx, workspace, a)
 	if err != nil {
-		return "", fmt.Errorf("getting path of bazel-bin: %w", err)
+		return "", fmt.Errorf("getting bazel tar path: %w", err)
 	}
 
-	tarPath := filepath.Join(bazelBin, buildTarPath(a.BuildTarget))
 	return tarPath, nil
 }
 
@@ -109,8 +107,17 @@ func (b *Builder) loadImage(ctx context.Context, out io.Writer, tarPath string,
 	return imageID, nil
 }
 
-func bazelBin(ctx context.Context, workspace string, a *latestV1.BazelArtifact) (string, error) {
-	args := []string{"info", "bazel-bin"}
+func bazelTarPath(ctx context.Context, workspace string, a *latestV1.BazelArtifact) (string, error) {
+	args := []string{
+		"cquery",
+		a.BuildTarget,
+		"--output",
+		"starlark",
+		// Bazel docker .tar output targets have a single output file, which is
+		// the path to the image tar.
+		"--starlark:expr",
+		"target.files.to_list()[0].path",
+	}
 	args = append(args, a.BuildArgs...)
 
 	cmd := exec.CommandContext(ctx, "bazel", args...)
@@ -133,13 +140,6 @@ func trimTarget(buildTarget string) string {
 	return trimmedTarget
 }
 
-func buildTarPath(buildTarget string) string {
-	tarPath := trimTarget(buildTarget)
-	tarPath = strings.Replace(tarPath, ":", string(os.PathSeparator), 1)
-
-	return tarPath
-}
-
 func buildImageTag(buildTarget string) string {
 	imageTag := trimTarget(buildTarget)
 	imageTag = strings.TrimPrefix(imageTag, ":")


Test patch:
diff --git a/pkg/skaffold/build/bazel/build_test.go b/pkg/skaffold/build/bazel/build_test.go
index e12d892d2..915676a8c 100644
--- a/pkg/skaffold/build/bazel/build_test.go
+++ b/pkg/skaffold/build/bazel/build_test.go
@@ -31,7 +31,9 @@ import (
 func TestBuildBazel(t *testing.T) {
 	testutil.Run(t, "", func(t *testutil.T) {
 		t.NewTempDir().Mkdir("bin").Chdir()
-		t.Override(&util.DefaultExecCommand, testutil.CmdRun("bazel build //:app.tar --color=no").AndRunOut("bazel info bazel-bin", "bin"))
+		t.Override(&util.DefaultExecCommand, testutil.CmdRun("bazel build //:app.tar --color=no").AndRunOut(
+			"bazel cquery //:app.tar --output starlark --starlark:expr target.files.to_list()[0].path",
+			"bin/app.tar"))
 		testutil.CreateFakeImageTar("bazel:app", "bin/app.tar")
 
 		artifact := &latestV1.Artifact{
@@ -67,15 +69,16 @@ func TestBuildBazelFailInvalidTarget(t *testing.T) {
 	})
 }
 
-func TestBazelBin(t *testing.T) {
+func TestBazelTarPath(t *testing.T) {
 	testutil.Run(t, "", func(t *testutil.T) {
 		t.Override(&util.DefaultExecCommand, testutil.CmdRunOut(
-			"bazel info bazel-bin --arg1 --arg2",
+			"bazel cquery //:skaffold_example.tar --output starlark --starlark:expr target.files.to_list()[0].path --arg1 --arg2",
 			"/absolute/path/bin\n",
 		))
 
-		bazelBin, err := bazelBin(context.Background(), ".", &latestV1.BazelArtifact{
-			BuildArgs: []string{"--arg1", "--arg2"},
+		bazelBin, err := bazelTarPath(context.Background(), ".", &latestV1.BazelArtifact{
+			BuildArgs:   []string{"--arg1", "--arg2"},
+			BuildTarget: "//:skaffold_example.tar",
 		})
 
 		t.CheckNoError(err)
@@ -83,14 +86,6 @@ func TestBazelBin(t *testing.T) {
 	})
 }
 
-func TestBuildTarPath(t *testing.T) {
-	buildTarget := "//:skaffold_example.tar"
-
-	tarPath := buildTarPath(buildTarget)
-
-	testutil.CheckDeepEqual(t, "skaffold_example.tar", tarPath)
-}
-
 func TestBuildImageTag(t *testing.T) {
 	buildTarget := "//:skaffold_example.tar"