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

feat: podman multiarch support #1157

Merged

Conversation

shashank381
Copy link
Contributor

No description provided.

Copy link

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

@kmehant kmehant changed the title Feat/podman multiarch support feat: podman multiarch support Mar 19, 2024
@github-actions github-actions bot added the feat label Mar 19, 2024
popd
{{- end }}

echo "done"

:SKIP
Copy link
Member

Choose a reason for hiding this comment

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

Add a new line at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I have added the new line.

@kmehant kmehant requested a review from Akash-Nayak March 19, 2024 08:42
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 14.82%. Comparing base (3bf7465) to head (3c96f4f).

❗ Current head 3c96f4f differs from pull request most recent head 99e8b8b. Consider uploading reports for the commit 99e8b8b to get more accurate results

Files Patch % Lines
common/utils.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1157      +/-   ##
==========================================
+ Coverage   14.79%   14.82%   +0.02%     
==========================================
  Files          90       90              
  Lines        8401     8385      -16     
==========================================
  Hits         1243     1243              
+ Misses       6835     6819      -16     
  Partials      323      323              

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

@HarikrishnanBalagopal HarikrishnanBalagopal added the lfx-project https://lfx.linuxfoundation.org/tools/mentorship/ label Mar 19, 2024
Copy link
Contributor

@Akash-Nayak Akash-Nayak left a comment

Choose a reason for hiding this comment

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

Few suggestions.

@@ -29,6 +29,10 @@ if not %basename% == "scripts" (
REM go to the parent directory so that all the relative paths will be correct
cd {{ .RelParentOfSourceDir }}

SET CONTAINER_RUNTIME={{ .ContainerRuntime }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the buildimages.bat and pushimages.bat file, we should now also allow the user to choose the container runtime while executing the buildandpushimages_multiarch.bat file.

:: 3) pushimages.bat index.docker.io your_registry_namespace podman

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we can also add an example on how to specify the container runtime (along with registry namespace/registry url/platforms) while executing the file .

:: 3) buildandpush_multiarchimages.bat quay.io your_quay_username linux/amd64,linux/arm64,linux/s390x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I have made the changes.

@@ -36,13 +36,28 @@ fi
if [ "$#" -eq 3 ]; then
PLATFORMS=$3
fi

CONTAINER_RUNTIME={{ .ContainerRuntime }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the buildimages.sh and pushimages.sh file, we should now also allow the user to choose the container runtime while executing the buildandpushimages_multiarch.sh file.

# 3) ./pushimages.sh index.docker.io your_registry_namespace podman

Copy link
Contributor

Choose a reason for hiding this comment

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

Here also we can add an example on how to specify the container runtime (along with registry namespace/registry url/platforms) while executing the file.

# 3) ./buildandpush_multiarchimages.sh quay.io your_quay_username linux/amd64,linux/arm64,linux/s390x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I have made the changes.

@Akash-Nayak
Copy link
Contributor

@shashank381 shashank381 force-pushed the feat/podman-multiarch-support branch 2 times, most recently from 1d8dbdf to bd679bf Compare March 21, 2024 14:01
@kmehant kmehant force-pushed the feat/podman-multiarch-support branch from bd679bf to 3c96f4f Compare March 22, 2024 06:52
Copy link
Contributor

@Akash-Nayak Akash-Nayak left a comment

Choose a reason for hiding this comment

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

Can you please also test the following scenarios and see if things are working as expected or not -

  1. ./buildandpush_multiarchimages.bat podman index.docker.io <your_quay_username> linux/amd64,linux/arm64,linux/s390x
  2. ./buildandpush_multiarchimages.bat docker index.docker.io <your_quay_username>
  3. ./buildandpush_multiarchimages.bat podman
  4. ./buildandpush_multiarchimages.bat


IF "%1"=="docker" (
shift
GOTO DOCKER_CONTAINER_RUNTIME
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this GOTO, the code till line 64 will get skipped and even if the user specifies registry url/ registry namespace/ platforms along with the container runtime while running the script, it will get ignored.

)
IF "%1"=="podman" (
shift
GOTO PODMAN_CONTAINER_RUNTIME
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this GOTO, the code till line 64 will get skipped and even if the user specifies registry url/ registry namespace/ platforms while running the script, it will get ignored.

GOTO PODMAN_CONTAINER_RUNTIME
)
SET CONTAINER_RUNTIME={{ .ContainerRuntime }}
GOTO DOCKER_CONTAINER_RUNTIME
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this GOTO also, the code till line 64 will get skipped and even if the user specifies registry url/ registry namespace/ platforms while running the script, it will get ignored.

Soumil-07 and others added 5 commits April 4, 2024 11:11
Signed-off-by: Soumil Paranjpay <soumil.paranjpay@gmail.com>
Signed-off-by: shashank381 <satholashashank@gmail.com>
Signed-off-by: shashank381 <satholashashank@gmail.com>
Signed-off-by: shashank381 <satholashashank@gmail.com>
Signed-off-by: shashank381 <satholashashank@gmail.com>
@shashank381 shashank381 force-pushed the feat/podman-multiarch-support branch from 3c96f4f to 99e8b8b Compare April 4, 2024 18:11
@shashank381
Copy link
Contributor Author

Thank you @Akash-Nayak I have updated the bat file.

Can you please also test the following scenarios and see if things are working as expected or not -

  1. ./buildandpush_multiarchimages.bat podman index.docker.io <your_quay_username> linux/amd64,linux/arm64,linux/s390x
  2. ./buildandpush_multiarchimages.bat docker index.docker.io <your_quay_username>
  3. ./buildandpush_multiarchimages.bat podman
  4. ./buildandpush_multiarchimages.bat

I dont have a windows machine to test these cases.

@shashank381
Copy link
Contributor Author

@Akash-Nayak I have manually validated logic for bat scripts. Thankyou.

Copy link
Contributor

@Akash-Nayak Akash-Nayak 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 @shashank381!

@Akash-Nayak Akash-Nayak merged commit f1a2483 into konveyor:main Apr 9, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat lfx-project https://lfx.linuxfoundation.org/tools/mentorship/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants