-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
use absolute paths to the container_image.tar in bazel build. #7671
Conversation
a8fdc30
to
add74a2
Compare
add74a2
to
1c4c6f6
Compare
Codecov Report
@@ 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
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Thanks for the PR @reltuk! Seems there is currently a Skaffold unit test failing on Windows related to this change:
Seems the abs path logic might not have some issues on Windows atm. Once we fix test we should be able to merge! |
@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! |
@tejal29 Thank you so much for looking at this while I’m traveling! Much appreciated :) |
…ContainerTools#7671) * fix: use absolute paths to the container_image.tar in bazel build. * fix unit tests Co-authored-by: tejal29 <tejal29@gmail.com>
* 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>
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 ifskaffold.yaml
is not in the workspace root andArtifact.Workspace
does not refer to the directory that contains the workspace symlinks. Previously, theArtifact.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
--symlink_prefix
passed to bazel will work.context:
is not set to navigate to the root of the bazel workspace will work.