feat: Use docker or podman to build and run #88

rmarting · 2021-09-30T07:11:10Z

This PR improves the Makefile to check which container tool is installed in the enviroment (only docker or podman).

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

github-actions · 2021-09-30T07:11:23Z

Thanks for making a pull request! 😃
One of the maintainers will review and advise on the next steps.

rmarting · 2021-09-30T07:14:38Z

@ashokponkumar Here the PR as you request me related with konveyor/move2kube-ui#98

Ready for your review.

rmarting · 2021-09-30T11:38:57Z

@HarikrishnanBalagopal I don't know why DCO is not passing. I followed the instructions to signoff my latest commit, but it is something not working.

From my local repo:

move2kube-api on  use-container-tool via 🐹 v1.16.8 via ⬢ v10.17.0 took 8s 
❯ git commit --amend --no-edit --signoff
[use-container-tool bfa4189] :twisted_rightwards_arrows: Merge branch 'use-container-tool' of https://github.com/rmarting/move2kube-api into use-container-tool
 Date: Thu Sep 30 12:06:19 2021 +0200
move2kube-api on  use-container-tool via 🐹 v1.16.8 via ⬢ v10.17.0 
❯ git push --force-with-lease origin use-container-tool
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 311 bytes | 311.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To https://github.com/rmarting/move2kube-api.git
 + c4d900a...bfa4189 use-container-tool -> use-container-tool (forced update)

What am I doing wrong?

ashokponkumar · 2021-09-30T12:18:37Z

@HarikrishnanBalagopal I don't know why DCO is not passing. I followed the instructions to signoff my latest commit, but it is something not working.

From my local repo:

move2kube-api on  use-container-tool via 🐹 v1.16.8 via ⬢ v10.17.0 took 8s 
❯ git commit --amend --no-edit --signoff
[use-container-tool bfa4189] :twisted_rightwards_arrows: Merge branch 'use-container-tool' of https://github.com/rmarting/move2kube-api into use-container-tool
 Date: Thu Sep 30 12:06:19 2021 +0200
move2kube-api on  use-container-tool via 🐹 v1.16.8 via ⬢ v10.17.0 
❯ git push --force-with-lease origin use-container-tool
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 311 bytes | 311.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To https://github.com/rmarting/move2kube-api.git
 + c4d900a...bfa4189 use-container-tool -> use-container-tool (forced update)

What am I doing wrong?

Possibly one of the commits ( I guess the merge commit) does not have the sign off. Possibly you can squash and sign that commit.

feat: update go version to 1.17 (#87) Signed-off-by: Harikrishnan Balagopal <[email protected]> :bug: Combine 3 commits into 1 :zap: Declared container tool to build and run Signed-off-by: Roman Martin <[email protected]> fix: config file paths were still using ids instead of normalized names (#89) Signed-off-by: Harikrishnan Balagopal <[email protected]> fix: config file paths were still using ids instead of normalized names (#89) Signed-off-by: Harikrishnan Balagopal <[email protected]> :zap: Declared container tool to build and run Signed-off-by: Roman Martin <[email protected]>

…es (#89) Signed-off-by: Harikrishnan Balagopal <[email protected]> Signed-off-by: Roman Martin <[email protected]>

rmarting · 2021-09-30T13:29:53Z

Possibly one of the commits ( I guess the merge commit) does not have the sign off. Possibly you can squash and sign that commit.

Squash some commits into single one, but now this PR includes my changes and others coming from the main branch. TBH I don't know what it is happeing, but at least the commits include my changes.

Ready for your review.

ashokponkumar · 2021-09-30T13:42:56Z

Possibly one of the commits ( I guess the merge commit) does not have the sign off. Possibly you can squash and sign that commit.

Squash some commits into single one, but now this PR includes my changes and others coming from the main branch. TBH I don't know what it is happeing, but at least the commits include my changes.

Ready for your review.

Can you please try a rebase with main? It shows a warning that the branch is out of date.

rmarting · 2021-09-30T13:47:11Z

Done!

ashokponkumar

Just a nit question.

ashokponkumar · 2021-09-30T15:29:44Z

Makefile

+ifdef DOCKER_CMD
+	${CONTAINER_TOOL} run --rm -p 8080:8080 -v /var/run/docker.sock:/var/run/docker.sock -v ${PWD}/:/workspace ${REGISTRYNS}/${BINNAME}:${VERSION}
+else
+	${CONTAINER_TOOL} run --rm -it -p 8080:8080 --network=bridge ${REGISTRYNS}/${BINNAME}:${VERSION}


@rmarting Do we need to run with -it flag for it to work?

No, we don't. I use it normally but TBH for this use case it could be not needed.

Would be good to remove it, since the api container will be running in non interactive mode.

It makes sense. Done!

ashokponkumar

Even within if block, the lines need to be tab spaced at start, otherwise it seems to get executed irrespective of whether the section is invoked or not.

Just a nit, just to make each section standout, we have refrained in general from using empty lines within each section in the makefile. If you think its okay, we can remove the empty line within the section.

ashokponkumar · 2021-09-30T16:09:16Z

Makefile

-	docker run --rm -p 8080:8080 -v /var/run/docker.sock:/var/run/docker.sock -v ${PWD}/:/workspace ${REGISTRYNS}/${BINNAME}:${VERSION}
+crun: ## Run container image
+ifndef CONTAINER_TOOL
+$(error No container tool (docker, podman) found in your environment. Please, install one)


This line needs a tab space at start

ashokponkumar · 2021-09-30T16:09:35Z

Makefile

-cpush: ## Push docker image
+cpush: ## Push container image
+ifndef CONTAINER_TOOL
+$(error No container tool (docker, podman) found in your environment. Please, install one)


This line needs a tab space at start

ashokponkumar · 2021-09-30T16:09:48Z

Makefile

-	docker tag ${REGISTRYNS}/${BINNAME}:${VERSION} ${REGISTRYNS}/${BINNAME}:latest
+cbuild: ## Build container image
+ifndef CONTAINER_TOOL
+$(error No container tool (docker, podman) found in your environment. Please, install one)


This line needs a tab space at start

Signed-off-by: Roman Martin <[email protected]>

github-actions bot added the feat label Sep 30, 2021

HarikrishnanBalagopal added 2 commits September 30, 2021 15:23

fix: config file paths were still using ids instead of normalized nam…

ecb0eb9

…es (#89) Signed-off-by: Harikrishnan Balagopal <[email protected]> Signed-off-by: Roman Martin <[email protected]>

Merge branch 'main' into use-container-tool

1bc49b8

ashokponkumar requested changes Sep 30, 2021

View reviewed changes

rmarting added 2 commits September 30, 2021 18:32

🐛 Added tabs in ifdef blocks

55d2b04

Signed-off-by: Roman Martin <[email protected]>

⚡ Removed interactive mode

0d6723c

Signed-off-by: Roman Martin <[email protected]>

ashokponkumar approved these changes Sep 30, 2021

View reviewed changes

ashokponkumar merged commit 86175a9 into konveyor:main Sep 30, 2021

rmarting deleted the use-container-tool branch September 30, 2021 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use docker or podman to build and run #88

feat: Use docker or podman to build and run #88

rmarting commented Sep 30, 2021

github-actions bot commented Sep 30, 2021

rmarting commented Sep 30, 2021

rmarting commented Sep 30, 2021

ashokponkumar commented Sep 30, 2021

rmarting commented Sep 30, 2021

ashokponkumar commented Sep 30, 2021

rmarting commented Sep 30, 2021

ashokponkumar left a comment

ashokponkumar Sep 30, 2021

rmarting Sep 30, 2021

ashokponkumar Sep 30, 2021

rmarting Sep 30, 2021

ashokponkumar left a comment

ashokponkumar Sep 30, 2021

rmarting Sep 30, 2021

ashokponkumar Sep 30, 2021

rmarting Sep 30, 2021

ashokponkumar Sep 30, 2021

rmarting Sep 30, 2021

feat: Use docker or podman to build and run #88

feat: Use docker or podman to build and run #88

Conversation

rmarting commented Sep 30, 2021

github-actions bot commented Sep 30, 2021

rmarting commented Sep 30, 2021

rmarting commented Sep 30, 2021

ashokponkumar commented Sep 30, 2021

rmarting commented Sep 30, 2021

ashokponkumar commented Sep 30, 2021

rmarting commented Sep 30, 2021

ashokponkumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokponkumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment