-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Use docker or podman to build and run the UI #98
feat: Use docker or podman to build and run the UI #98
Conversation
Thanks for making a pull request! 😃 |
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.
Some minor changes required. Other than that LGTM.
Thanks for the PR @rmarting ! Once this PR is merged, do you mind creating similar PRs for the following repos too : https://github.com/konveyor/move2kube/blob/main/Makefile |
Thanks @ashokponkumar for your comments! I could do it in the other repos as soon this PR has been reviewed and accepted with the minor changes. Then I will be sure which changes I have to do in these repositories. |
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.
Could you review it with my latest changes?
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
Makefile
Outdated
CONTAINER_TOOL = 'podman' | ||
endif | ||
|
||
ifndef CONTAINER_TOOL |
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.
Can we move this error to the targets that actually require it? make cbuild
and make crun
make install
doesn't need it for instance.
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.
Also I have rebased the branch on top of main, we are using rebases instead of merges in this repo's commit history.
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.
Done! WDTY?
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.
@rmarting Thanks for making all the requested changes.
I am just debugging why make
is saying it can't find command
Seems other people had the same issue cnabio/duffle#502
Will merge after I fix this.
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.
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.
📝 Updated how to start UI ⚡ Checked container runtime to start container 🐛 Used switch command to get container tool path ♻️ Fixed PR comments ⚡ Removed data folder creation ⏪ Used command to check container tool 🎨 Check errors in the right place Signed-off-by: Roman Martin <[email protected]>
See https://stackoverflow.com/questions/12989869/calling-command-v-find-from-gnu-makefile/12991757 and https://stackoverflow.com/questions/5618615/check-if-a-program-exists-from-a-makefile Signed-off-by: Harikrishnan Balagopal <[email protected]>
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 again
Thank you so much for your support and review! Glad to contribute! On the next days I will try to clone this changes in the other repos. |
Thanks @rmarting ! |
This PR improves the
Makefile
to check which container tool is installed in the enviroment (onlydocker
orpodman
) related with the issue #97.The user will not have to set up anything before to build or to run the image locally, only a container tool is needed to be installed.
HTH