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

Add docker builder option to provider #987

Merged

Conversation

aacebedo
Copy link
Contributor

@aacebedo aacebedo commented Apr 7, 2024

This PR solves #984 by first checking if podman is used when buildxExists is called and return true in this case.
It also adds a DOCKER_BUILDER option to the provider in order to choose easily between the 2 builders.

@aacebedo
Copy link
Contributor Author

aacebedo commented Apr 7, 2024

@pascalbreuninger can you have a look to this one and give me some feedback please?

Copy link
Member

@pascalbreuninger pascalbreuninger left a comment

Choose a reason for hiding this comment

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

@aacebedo thanks for the PR! Got a couple of comments and want to make sure tests are still working

cmd/build.go Outdated Show resolved Hide resolved
pkg/docker/helper.go Outdated Show resolved Hide resolved
@aacebedo
Copy link
Contributor Author

aacebedo commented Apr 9, 2024

@aacebedo thanks for the PR! Got a couple of comments and want to make sure tests are still working

Thanks for the review! Yes I will solve the e2e tests.

@aacebedo aacebedo force-pushed the aacebedo/add_docker_builder_option branch 3 times, most recently from ec49f97 to 7a60471 Compare April 20, 2024 09:30
@aacebedo
Copy link
Contributor Author

@aacebedo thanks for the PR! Got a couple of comments and want to make sure tests are still working

Thanks for the review! Yes I will solve the e2e tests.

@pascalbreuninger I have fixed the test. I have added a workflow dispatch event to E2E test to ease the next PRs. Don't know if it is ok for you but it helped me a lot as I do not have the same environment as the CI.

)

func (db DockerBuilder) String() string {
return [...]string{"buildkit", "buildx", ""}[db]
Copy link
Member

Choose a reason for hiding this comment

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

last small issue: since the default is now "", the order should probably be updated as well

Suggested change
return [...]string{"buildkit", "buildx", ""}[db]
return [...]string{"", "buildx", "buildkit"}[db]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

@pascalbreuninger
Copy link
Member

@pascalbreuninger I have fixed the test. I have added a workflow dispatch event to E2E test to ease the next PRs. Don't know if it is ok for you but it helped me a lot as I do not have the same environment as the CI.

One small cleanup remaining and we're good to merge.
The workflow dispatch makes sense 👍 Also just doubled checked and apparently only collaborators and above can trigger it in the main repo. Just wanted to be sure we're not opening an avenue for burning through our Github Action minutes 😅

@aacebedo aacebedo force-pushed the aacebedo/add_docker_builder_option branch from 7a60471 to 6389e94 Compare April 20, 2024 11:17
Copy link
Member

@pascalbreuninger pascalbreuninger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks 🙌

@pascalbreuninger pascalbreuninger merged commit ca73d7c into loft-sh:main Apr 20, 2024
25 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.

2 participants