Implementation for podman to be used for container images #1357

Jdubrick · 2023-11-20T21:00:10Z

What does this PR do?

This PR enables the use of podman for container images instead of explicitly docker.

Which issue(s) does this PR fix

resolves #1341

PR acceptance criteria

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.

Open new test/doc issues under the devfile/api repo

Check each criteria if:

There is a separate tracking issue. Add the issue link under the criteria
or

test/doc updates are made as part of this PR

If unchecked, explain why it's not needed

Implement podman support for devfile/api build and run scripts.
Update documentation to reflect changes in the build/deploy process.
Sufficiently test changes made.

How to test changes / Special notes to the reviewer

Run bash ./docker-run.sh ./build.sh with export USE_PODMAN=true for podman testing and export USE_PODMAN=false for docker.

Signed-off-by: Jordan Dubrick <[email protected]>

yangcao77 · 2023-11-20T22:15:33Z

README.md

@@ -34,8 +34,8 @@ Release details and process are found in [Devfile Release](RELEASE.md)

 ## How to build


this section is duplicating the content in contributing doc. We shouldn't spent effort to maintain same content in separate docs. Please use the link to contributing.md, e.g. ./CONTRIBUTING.md#building

I agree about the duplication :) Thinking that in general the contribution guidelines are more related to information about how someone could open a PR or an issue, I was wondering if the contributing.md is the right file to maintain this section? WDYT?

Thank you for pointing that out! To remove the duplication I agree with contributing.md probably not being the right spot for the build instructions, I'll remove it from contributing and keep in in the readme.md only

@yangcao77 @thepetk please see the latest commit for my updated changes.

should directly link to the building section. not the top of the contributing doc

yangcao77

generally looks good, left a comment on the doc

Signed-off-by: Jordan Dubrick <[email protected]>

codecov · 2023-11-21T15:55:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (adb72c3) 35.75% compared to head (d086125) 35.75%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1357   +/-   ##
=======================================
  Coverage   35.75%   35.75%           
=======================================
  Files          52       52           
  Lines        6696     6696           
=======================================
  Hits         2394     2394           
  Misses       4158     4158           
  Partials      144      144

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

amisevsk · 2023-11-21T18:27:20Z

docker-run.sh

+# Run script to set env var for podman if necessary
+. ./setenv.sh
+


I'm not sure I like hiding the alias in a separate script and controlled by an env var -- it's hiding a lot of functionality here (if you don't notice these two lines at the top it looks like it just uses docker).

amisevsk · 2023-11-21T18:27:38Z

setenv.sh

+podman=${USE_PODMAN:-false}
+if [ ${podman} == true ]; then
+  alias docker=podman
+  echo "setting alias docker=podman"
+fi


Why not inline this?

I do have the same question, why using a separate script just for setting alias?

The reasoning for having it in a separate script was only because I was going off of prior work done here by someone else. That repo utilizes the same setenv.sh script being used alongside an env variable to toggle between podman or docker.

If it's preferred to be inline that change can definitely be made.

The way I've usually done it in other repos is to just use a variable, e.g.

DOCKER=docker if [ <use podman condition> ]; then DOCKER=podman fi $DOCKER build -t <image> -f <file> <context>

this also avoids the need for setting shopt -s expand_alias.

In my last commit I added the setting of the alias inline so we don't need another script to be doing that work. I initially tried to go with your way @amisevsk with the use of a variable but found that when it was trying to call the ./build.sh script it was failing with a seg fault, this was not happening with setting the alias. I'm not sure why so more investigation will have to be done into that if we want to not use an alias and rather a variable.

Signed-off-by: Jordan Dubrick <[email protected]>

yangcao77

look good to me. thx for the changes

openshift-ci · 2023-11-22T19:49:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, Jdubrick, yangcao77

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Jdubrick added 3 commits November 20, 2023 14:13

add setenv script

60baa9e

Signed-off-by: Jordan Dubrick <[email protected]>

update run script to include podman capabilities

1e61932

Signed-off-by: Jordan Dubrick <[email protected]>

update documentation to reflect podman implementation

3870586

Signed-off-by: Jordan Dubrick <[email protected]>

Jdubrick requested review from amisevsk, yangcao77 and johnmcollier as code owners November 20, 2023 21:00

yangcao77 reviewed Nov 20, 2023

View reviewed changes

update documentation to remove duplicate entries

cce5fe4

Signed-off-by: Jordan Dubrick <[email protected]>

amisevsk approved these changes Nov 21, 2023

View reviewed changes

openshift-ci bot assigned amisevsk Nov 21, 2023

openshift-ci bot added the lgtm label Nov 21, 2023

Merge branch 'devfile:main' into podman-implementation

965bdcd

openshift-ci bot removed the lgtm label Nov 22, 2023

Jdubrick added 2 commits November 22, 2023 14:08

add inline setting of alias to remove need for additional script

f2d6c95

Signed-off-by: Jordan Dubrick <[email protected]>

fix unset variable

d086125

Signed-off-by: Jordan Dubrick <[email protected]>

yangcao77 approved these changes Nov 22, 2023

View reviewed changes

openshift-ci bot assigned yangcao77 Nov 22, 2023

openshift-ci bot added the lgtm label Nov 22, 2023

Jdubrick merged commit e37cd6b into devfile:main Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implementation for podman to be used for container images #1357

Implementation for podman to be used for container images #1357

Uh oh!

Jdubrick commented Nov 20, 2023

Uh oh!

yangcao77 Nov 20, 2023 •

edited

Loading

Uh oh!

thepetk Nov 21, 2023

Uh oh!

Jdubrick Nov 21, 2023

Uh oh!

Jdubrick Nov 21, 2023

Uh oh!

yangcao77 Nov 21, 2023

Uh oh!

yangcao77 left a comment •

edited

Loading

Uh oh!

codecov bot commented Nov 21, 2023 •

edited

Loading

Uh oh!

amisevsk Nov 21, 2023

Uh oh!

amisevsk Nov 21, 2023

Uh oh!

yangcao77 Nov 21, 2023

Uh oh!

Jdubrick Nov 21, 2023

Uh oh!

amisevsk Nov 22, 2023 •

edited

Loading

Uh oh!

Jdubrick Nov 22, 2023 •

edited

Loading

Uh oh!

yangcao77 left a comment

Uh oh!

openshift-ci bot commented Nov 22, 2023

Uh oh!

Uh oh!

		@@ -34,8 +34,8 @@ Release details and process are found in [Devfile Release](RELEASE.md)

		## How to build

		# Run script to set env var for podman if necessary
		. ./setenv.sh

Implementation for podman to be used for container images #1357

Implementation for podman to be used for container images #1357

Uh oh!

Conversation

Jdubrick commented Nov 20, 2023

What does this PR do?

Which issue(s) does this PR fix

PR acceptance criteria

How to test changes / Special notes to the reviewer

Uh oh!

yangcao77 Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangcao77 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amisevsk Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jdubrick Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangcao77 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 22, 2023

Uh oh!

Uh oh!

yangcao77 Nov 20, 2023 •

edited

Loading

yangcao77 left a comment •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

amisevsk Nov 22, 2023 •

edited

Loading

Jdubrick Nov 22, 2023 •

edited

Loading