Support bake remote definitions #656
Conversation
|
Many thanks for the PR! I'll take a look when I have the time :) |
| config = docker.buildx.bake( | ||
| print=only_print, | ||
| context="https://github.com/gabrieldemarmiesse/python-on-whales.git#v0.74.0:tests/python_on_whales/components/bake_tests", | ||
| ) |
There was a problem hiding this comment.
Can you also add a test where you specify a target to make sure the arguments are passed in the right order?
| @@ -126,6 +126,7 @@ def bake( | |||
| set: Dict[str, str] = {}, | |||
| variables: Dict[str, str] = {}, | |||
| stream_logs: bool = False, | |||
| context: Union[str, None] = None, | |||
There was a problem hiding this comment.
The issue I have with context is that context already exists: https://docs.docker.com/build/bake/contexts/ and it's slight different.
What about being explicit? Unless I'm missing something, we can just use "remote_definition", it's the term used in the docker documentation.
There was a problem hiding this comment.
Yeah remote_definition seems good, changed to that.
The interesting behavior with context is that by default, the contents of the remote_context become the value of Bake target contexts (not contexts), for example in the test definition. However, this is only the default; if the Bake file defines a context, it may be unrelated to this value.
TBH I think the whole idea of this Bake functionality needs some clarification/cleanup from Docker folks, but until that happens I think this is the best we can do!
| @@ -22,6 +22,10 @@ jobs: | |||
| cp docs/mkdocs.yml ./ | |||
| - name: Render | |||
| run: uv run mkdocs build | |||
| - uses: actions/upload-artifact@v4 | |||
There was a problem hiding this comment.
Not mandatory, but since this is an awesome feature, you can add it to the CONTRIBUTING.md :) so contributors don't have to build the docs themselves. Just say
- where to click to get the files
- unzip it
- open index.html with the browser
Again it's optional since you contributed this feature and it's a net improvement, but if you want to go the extra mile, I'd appreciate a note in the contributing guide.
rcwbr
force-pushed
the
support-bake-remote-definitions
branch
from
c750635 to
2e51334
Compare
Thank you, and thanks for the awesome library!!! Should be ready for re-review now if you'd like |
gabrieldemarmiesse
merged commit b2ff865
into
gabrieldemarmiesse:master
Docker buildx bake supports remote Bake file definitions, but this functionality seems to be missing from the existing API. This change intends to add support for this.
I was lazy about rendering the docs, so also threw in a change to save the rendered files from the CI pipeline as an artifact -- not sure if this would have too much impact on GitHub storage, but figured I'd propose the change.
A couple notes on the implementation:
context" -- "definition" or "target" might align better to upstream docs, though neither is clearly specified in docs and all are overloaded 😬 Very happy to change to a better name if there is one!contextarg is directlyappended to thefull_cmd, meaning the content of the arg could technically be any valid bake CLI arg. Validation could prevent this, but didn't seem too important.v0.74.0tag. This seems slightly awkward, but should at least be safe since its immutable.Please let me know if I'm missing anything, or if anything needs to be changed!