-
Notifications
You must be signed in to change notification settings - Fork 220
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
Add --output option to devcontainer build #166
Add --output option to devcontainer build #166
Conversation
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Left a few comments for changes / discussion.
Both requested changes have been committed. Hopefully the test works; I was not able to get yarn test to work in my local env for reasons I couldn't quite figure out. |
Previous test failed due to unbalanced quotes, looks like. Latest commit should fix it. |
@natescherer There are some test failures. Could you look into these? The change looks good otherwise. |
Looks like I was using a non-Dockerfile config to try and run the test which wouldn't work. Changed it and hopefully the test will pass now! |
@chrmarti Could you please trigger the tests again when you get a minute? Looks like they don't automatically run, probably since I'm a first-time contributor to this repo. |
Looks like synchronizing from main caused there to be a missing comma. Last commit should fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@chrmarti Looks like the local environment on the machine running the tests was using the default docker buildx driver. Added wrapper to create a new buildx driver at the start of the test and then reset to the default driver after. Hopefully this will get that test to pass now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@natescherer looks like there are some merge conflicts. Could you help resolve them? thanks!
@samruddhikhandale I re-merged main with my branch, should be good now hopefully! |
@natescherer Merged, thanks! |
This is related to what was discussed in #155.
This PR adds new options
--output-type
and--output-dest
that pass through todocker buildx build --output
.If there's anything that needs added or changed, please let me know!