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

use absolute paths to the container_image.tar in bazel build. #7671

Conversation

reltuk
Copy link
Contributor

@reltuk reltuk commented Jul 21, 2022

Fixes: #7600
Related: #7251, #7281, #7430

Description

This PR changes the bazel build task to get an absolute path to the built container image. The correct path is the concatenation of the bazel execution_root and the result of the cquery for the output path of the configured target.

As per #7600, the current approach is broken if workspace symlinks are disabled. It's actually also broken if workspace symlinks are anything other than bazel-. The current approach is also broken if skaffold.yaml is not in the workspace root and Artifact.Workspace does not refer to the directory that contains the workspace symlinks. Previously, the Artifact.Workspace did not need to refer to the root of the workspace for a bazel build target, since the target can be an absolute path and it will build regardless of where bazel is invoked.

I believe using the absolute path is more robust and handles more use cases than the current approach.

User facing changes

  • skaffold bazel builds and pushes with --symlink_prefix passed to bazel will work.
  • skaffold bazel builds and pushes in directories that are not the root of the bazel workspace and where context: is not set to navigate to the root of the bazel workspace will work.

@reltuk reltuk force-pushed the bazel-build-no-symlink-use-execution_root branch from a8fdc30 to add74a2 Compare July 21, 2022 18:51
@reltuk reltuk force-pushed the bazel-build-no-symlink-use-execution_root branch from add74a2 to 1c4c6f6 Compare July 21, 2022 18:53
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #7671 (53223ca) into main (290280e) will decrease coverage by 3.79%.
The diff coverage is 53.87%.

@@            Coverage Diff             @@
##             main    #7671      +/-   ##
==========================================
- Coverage   70.48%   66.68%   -3.80%     
==========================================
  Files         515      587      +72     
  Lines       23150    28228    +5078     
==========================================
+ Hits        16317    18824    +2507     
- Misses       5776     8033    +2257     
- Partials     1057     1371     +314     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/flags.go 93.00% <ø> (+2.18%) ⬆️
cmd/skaffold/app/cmd/test.go 44.44% <0.00%> (ø)
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/render.go 35.48% <18.18%> (-5.90%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/run.go 64.28% <33.33%> (-9.63%) ⬇️
cmd/skaffold/app/cmd/fix.go 56.41% <35.71%> (-20.07%) ⬇️
... and 346 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@aaron-prindle
Copy link
Contributor

aaron-prindle commented Jul 26, 2022

Thanks for the PR @reltuk! Seems there is currently a Skaffold unit test failing on Windows related to this change:

--- FAIL: TestBazelTarPath (0.03s)
    --- FAIL: TestBazelTarPath/EmptyExecutionRoot (0.02s)
        build_test.go:10[8](https://github.com/GoogleContainerTools/skaffold/runs/7527004738?check_suite_focus=true#step:6:9): string differ (-got, +want):   string(
            - 	`\absolute\path\bin`,
            + 	"/absolute/path/bin",
              )
    --- FAIL: TestBazelTarPath/AbsoluteExecutionRoot (0.02s)
        build_test.go:[12](https://github.com/GoogleContainerTools/skaffold/runs/7527004738?check_suite_focus=true#step:6:13)2: string differ (-got, +want):   strings.Join({
            - 	`\var\tmp\`,
            + 	"/var/tmp/",
              	"bazel-execution-roots",
            - 	`\abcdefg\execroot\workspace_name\bazel-bin\`,
            + 	"/abcdefg/execroot/workspace_name/bazel-bin/",
              	"darwin-fastbuild-ST-confighash",
            - 	`\path\to\`,
            + 	"/path/to/",
              	"bin",
              }, "")

Seems the abs path logic might not have some issues on Windows atm. Once we fix test we should be able to merge!

@reltuk
Copy link
Contributor Author

reltuk commented Jul 29, 2022

@aaron-prindle Thanks for the feedback and for bringing the test failure on windows to my attention. I think filepath.Join is normalizing the path separator, but the mocked data in the test setup does not take that behavior into account. I suspect adding a normalizing call in the test setup will fix it. I’m currently traveling but will hopefully be able to fix this soon.

Thanks again!

@reltuk
Copy link
Contributor Author

reltuk commented Aug 3, 2022

@tejal29 Thank you so much for looking at this while I’m traveling! Much appreciated :)

@tejal29 tejal29 merged commit 2f225e3 into GoogleContainerTools:main Aug 3, 2022
mikekamornikov pushed a commit to mikekamornikov/skaffold that referenced this pull request Sep 18, 2022
…ContainerTools#7671)

* fix: use absolute paths to the container_image.tar in bazel build.

* fix unit tests

Co-authored-by: tejal29 <tejal29@gmail.com>
gsquared94 pushed a commit that referenced this pull request Sep 21, 2022
* fix: running skaffold from a directory other than repository root (#7430)

* use absolute paths to the container_image.tar in bazel build. (#7671)

* fix: use absolute paths to the container_image.tar in bazel build.

* fix unit tests

Co-authored-by: tejal29 <tejal29@gmail.com>

Co-authored-by: Sergei Morozov <morozov@tut.by>
Co-authored-by: Aaron Son <aaron@dolthub.com>
Co-authored-by: tejal29 <tejal29@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold cannot find image tar with bazel symlinks disabled
3 participants