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.

Sign up for GitHub

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

Jump to bottom

feat(build): support for multi arch container image #128

Merged
merged 2 commits into from
Nov 10, 2020
Merged

feat(build): support for multi arch container image #128

merged 2 commits into from
Nov 10, 2020

Conversation

prateekpandey14
Copy link
Contributor

Signed-off-by: prateekpandey14 prateek.pandey@mayadata.io

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • PR messages has document related information
  • Labelled this PR & related issue with documentation tag
  • PR messages has breaking changes related information
  • PR messages has upgrade related information
  • Labelled this PR & related issue with requires-upgrade tag
  • Tests updated

@prateekpandey14 prateekpandey14 force-pushed the multi-arch branch 2 times, most recently from abcecb2 to 5476cdf Compare November 3, 2020 13:43
@codecov-io
Copy link

codecov-io commented Nov 3, 2020 •
edited

Codecov Report

Merging #128 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #128   +/-   ##
=======================================
  Coverage   42.47%   42.47%           
=======================================
  Files          12       12           
  Lines         412      412           
=======================================
  Hits          175      175           
  Misses        224      224           
  Partials       13       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdae73d...1b029a3. Read the comment docs.

@prateekpandey14 prateekpandey14 force-pushed the multi-arch branch 2 times, most recently from e414b75 to 99ed8ce Compare November 3, 2020 19:01
@shubham14bajpai shubham14bajpai mentioned this pull request Nov 4, 2020
Closed
@prateekpandey14
feat(build): support for multi arch container image
099d5c0 
Signed-off-by: prateekpandey14 <prateek.pandey@mayadata.io>
@prateekpandey14 prateekpandey14 added this to RC1 - Due: Nov 5 2020 in 2.3 Release Tracker - Due Nov 15th. Nov 4, 2020
akhilerm
akhilerm reviewed Nov 4, 2020
View reviewed changes
.travis.yml Show resolved Hide resolved
akhilerm
akhilerm reviewed Nov 4, 2020
View reviewed changes
Makefile.buildx.mk Outdated Show resolved Hide resolved
akhilerm
akhilerm reviewed Nov 4, 2020
View reviewed changes
Makefile.buildx.mk Outdated

# default list of platforms for which multiarch image is built
ifeq (${PLATFORMS}, )
export PLATFORMS="linux/amd64,linux/arm64,linux/ppc64le"
Copy link
Contributor

Choose a reason for hiding this comment

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

@shubham14bajpai , for csi-driver also do we have issue with arm/v7

Copy link
Contributor

Choose a reason for hiding this comment

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

Without the libcstor builds we will not be able to test arm/v7 for any other cstor image so waiting on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added arm/v7

akhilerm
akhilerm reviewed Nov 4, 2020
View reviewed changes
buildscripts/build.sh
@@ -34,7 +34,7 @@ else
fi

# Set BUILDMETA based on travis tag
if [[ -n "$TRAVIS_TAG" ]] && [[ $TRAVIS_TAG != *"RC"* ]]; then
if [[ -n "$RELEASE_TAG" ]] && [[ $RELEASE_TAG != *"RC"* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

akhilerm
akhilerm reviewed Nov 4, 2020
View reviewed changes
buildscripts/cstor-csi-driver/cstor-csi-driver.Dockerfile

RUN make buildx.csi-driver

FROM ubuntu:16.04
Copy link
Contributor

Choose a reason for hiding this comment

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

@prateekpandey14 Is it necessary to use ubuntu image? Can we switch to alpine? I am thinking we should slim down the images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is tricky ..using alpine can cause some other issues and install pkg availabilty like xfs etc, i may try with ubuntu latest release which is more trimmed container images as compared to 16.04

akhilerm
akhilerm reviewed Nov 4, 2020
View reviewed changes
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@prateekpandey14 Have given some comments.

@akhilerm
Copy link
Contributor

akhilerm commented Nov 5, 2020

@prateekpandey14 Changes looks good. Can you check why travis failed? Also the pull request action was not executed.

akhilerm
akhilerm approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

/lgtm

akhilerm
akhilerm reviewed Nov 9, 2020
View reviewed changes
.github/workflows/build.yml Outdated
Comment on lines 81 to 82
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

akhilerm
akhilerm reviewed Nov 9, 2020
View reviewed changes
.github/workflows/release.yml Outdated
Comment on lines 49 to 50
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

akhilerm
akhilerm suggested changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@prateekpandey14 A change is required for the docker credential env.

@prateekpandey14
remove extra build args and fix review comments
38a54bf 
Signed-off-by: prateekpandey14 <prateek.pandey@mayadata.io>
akhilerm
akhilerm approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

@kmova kmova merged commit 8110a6f into openebs-archive:master Nov 10, 2020
2.3 Release Tracker - Due Nov 15th. automation moved this from RC1 - Due: Nov 5 2020 to Done Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akhilerm akhilerm akhilerm approved these changes

@shubham14bajpai shubham14bajpai shubham14bajpai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@prateekpandey14 @codecov-io @akhilerm @shubham14bajpai @kmova