Skip to content

Conversation

srajmane
Copy link
Contributor

@srajmane srajmane commented Apr 7, 2020

As Travis CI officially supports s390x builds, adding support for same.

Signed-off-by: Snehal Rajmane <srajmane@us.ibm.com>
.travis.yml Outdated
include:
- arch: amd64
- arch: s390x
dist: bionic

Choose a reason for hiding this comment

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

I don't think we should test one on trusty and one on bionic. Tests could error because of differences between those two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

s390x build runs within an LXD container on Z-based infrastructure. LXD containers are with (Bionic), Xenial only.
Reference: https://docs.travis-ci.com/user/reference/overview/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuperSandro2000 Could you please have a look ?

Choose a reason for hiding this comment

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

I have nothing to say here but a good approach would be to update the other tests also to bionic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuperSandro2000 Added dist: bionic for amd64 and s390x. Also set GO111MODULE=on to resolve l.LintPkg undefined (type *"github.com/golangci/lint-1".Linter has no field or method LintPkg) error.
Ref: https://github.com/golangci/golangci-lint#go

Signed-off-by: Snehal Rajmane <srajmane@us.ibm.com>
@srajmane
Copy link
Contributor Author

srajmane commented May 12, 2020

@dmcgowan @caervs @davidswu Could you please have a look ?

@srajmane
Copy link
Contributor Author

@thaJeztah @dmcgowan Please have a look.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM (not a maintainer)

@manishtomar @dmcgowan PTAL

@manishtomar
Copy link
Contributor

@thaJeztah What is our policy in general for other arch support? Do you think its sufficient to just have CI enabled?

@thaJeztah
Copy link
Member

I don't think there's an official "policy" for this repository ( we are working with the IBM Power and Z teams on docker packages for those platforms, and we have CI running for those in the upstream moby repository)

I don't think it hurts to have CI running to check if nothing breaks on those architectures

@thaJeztah
Copy link
Member

Perhaps arm64 should be enabled as well if it doesn't considerably slow down testing on PR's

Copy link
Contributor

@manishtomar manishtomar left a comment

Choose a reason for hiding this comment

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

LGTM

@manishtomar manishtomar merged commit 1d0ea8e into distribution:master Jul 30, 2020
@srajmane
Copy link
Contributor Author

Hi @olegburov , added s390x support to make sure nothing breaks for s390x.

@ob1dev
Copy link
Contributor

ob1dev commented Aug 24, 2020

Sorry, I meant why does the Registry care of s390x? Is this architecture it must to support?

@manishtomar
Copy link
Contributor

@olegburov I don't think this repo provides "official" support of s390x by having it in CI. If any user has problems with that arch they are welcome to create a ticket or open a PR. It is a different matter whether it will be merged or worked on. Like @thaJeztah said it doesn't hurt adding the CI validation.

@ob1dev
Copy link
Contributor

ob1dev commented Aug 24, 2020

Got it. The reason why I asked is, if we want to switch from Travis CI to GitHub Actions in some future, we will have to drop s390x architecture, because it doesn't support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants