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

439 no support for buildx build build context #440

Conversation

bigcat2014
Copy link
Contributor

Fixes #439

@gabrieldemarmiesse
Copy link
Owner

Sorry it seems there is an issue with the CI. I'll try to fix it quickly. Thanks for your patience! In the meantime, could you add a simple unit test for this new feature?

@bigcat2014
Copy link
Contributor Author

Working on unit tests at the moment, although this is my first contribution to open source and I've never had to do unit tests before, so it's taking me a min to work through the structure of them. Should be able to have something to review soon though.

@bigcat2014
Copy link
Contributor Author

So, with the current unit tests, 2 of the ones that I added are expected to fail until it is merged into master because they use absolute paths to files that exist on the branch, which will become master once this is merged (references a test_context directory that I created in the git repository and a .tar file)

@gabrieldemarmiesse
Copy link
Owner

gabrieldemarmiesse commented May 25, 2023

No worries, I'll fix it, give me some time :) and sorry for the wait

@bigcat2014
Copy link
Contributor Author

@gabrieldemarmiesse Any movement on this recently? Anything I can update in the unit tests to move this along more quickly?

@gabrieldemarmiesse
Copy link
Owner

yeah I'm sorry, this pull request fell at the bottom of the list. Thanks for reminding me. It seems the tests are not trivial so I think I'll handle them myself. I'll likely have some time tomorrow afternoon to work on it. Thanks for your patience!

@bigcat2014
Copy link
Contributor Author

I've updated some of the unit tests, adding a test for oci layout compliant directories and removing the test for .tar files as they're not explicitly supported by the --build-context flag. The only test that's still a little goofy to implement is the git repository one, but I think what I have is at least a good starting point for you!

@gabrieldemarmiesse
Copy link
Owner

thanks I'll try it out :)

@gabrieldemarmiesse
Copy link
Owner

@bigcat2014 do you need help?

@bigcat2014
Copy link
Contributor Author

bigcat2014 commented Jul 20, 2023

I'm not sure, I keep running the unit tests locally, they pass, then they don't pass the automated checks once I push, so I'm not sure exactly what's going on. If the current ones don't pass then I may need you to take a look

@bigcat2014
Copy link
Contributor Author

Ok, the unit tests I have added seem to be passing now. I think it was an issue with my understanding of the "with_docker_driver" and "with_container_driver" fixtures. I updated my tests to use what I think are the correct fixtures now and it seems to be working. I think I've also covered all of the --build-context use cases (local path, git repo, oci layout compliant directory, and docker image)

@bigcat2014
Copy link
Contributor Author

Ok, I'm not entirely sure what's going on. I had all of the unit tests passing except the linting, I ran the linter to fix formatting. It only updated the test_buildx_cli_wrapper.py file and only updated formatting of the unit tests I added, but now a different unit test that I did not add, which was not reformatted at all, is failing.

Copy link
Owner

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few changes and we're good to go!

@gabrieldemarmiesse
Copy link
Owner

We do have some flaky tests here and there, so it's possible that some tests fail randomly :(

@bigcat2014
Copy link
Contributor Author

I've implemented all your suggestions and all unit tests seem to be passing, should be good to go! Let me know if there's anything else you see that I would need to change/update!

@gabrieldemarmiesse
Copy link
Owner

thanks a lot! It's going to help a lot of users :)

@gabrieldemarmiesse gabrieldemarmiesse merged commit 9d79f88 into gabrieldemarmiesse:master Jul 20, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No support for buildx build --build-context ...
2 participants