Skip to content

Conversation

@fgiudici
Copy link

@fgiudici fgiudici commented Sep 25, 2020

Update the ubuntu version used in cri-o-release-1.11 branch to a supported one.
Cherry-pick #4f49520d32f91aa02fe04623d85d89af86845e7c

If for any reason we want to stick to ubuntu:artful here, we may instead update the repo sources before apt-getting the packages:
sed -i 's/archive.ubuntu.com/old-releases.ubuntu.com/g' /etc/apt/sources.list


Use the latest stable version of Ubuntu, since Ubuntu 17.10 (i.e.,
ArtfulAardvark) hit EOL.

Fixes: #648
Signed-off-by: Valentin Rothberg [email protected]

Use the latest stable version of Ubuntu, since Ubuntu 17.10 (i.e.,
ArtfulAardvark) hit EOL.

Fixes: #648
Signed-off-by: Valentin Rothberg <[email protected]>

----
Cherry-picked from #4f49520d32f91aa02fe04623d85d89af86845e7c
Signed-off-by: Francesco Giudici <[email protected]>
@TomSweeneyRedHat
Copy link
Member

Tests in this repo do not like you @fgiudici ....

@TomSweeneyRedHat
Copy link
Member

Looks like:

vendor/github.com/containers/storage/pkg/archive/archive_zstd.go:6:2: cannot find package "github.com/klauspost/compress/zstd" in any of:

can't be found. Was this removed from Ubuntu 18.04 or are other steps needed?

@fgiudici
Copy link
Author

fgiudici commented Sep 28, 2020

Tests in this repo do not like you @fgiudici ....

lol, think you are right! :-)
Giving it another try...

@fgiudici fgiudici marked this pull request as draft September 28, 2020 11:55
current master branch of the storage package has different dependencies
than the cri-o-release-11 version of the image package. Use a storage
version targeting the same older stable releases for CI testing.
Update also github.com/opencontainers/selinux pkg to a more recent
version compatible with the imported one from storage.

Signed-off-by: Francesco Giudici <[email protected]>
@mtrmac
Copy link
Collaborator

mtrmac commented Sep 28, 2020

A quick drive-by comment: Rather than update formatting to conform to some recent version, it would be cleaner to freeze the Go or gofmt version back to the version at the branching time, to minimize the number of changes on the branch (and indirectly in CRI-O.). (OTOH I haven’t checked whether that’s even possible.)

We want to avoid changes as much as possible in this old and stable
branch. Let's just skip the gofmt check for the docker/tarfile/src.go,
which would fail on the alphabetical order of the imports.
Goal is to have "make validate" and CI not failing on the 1.11 branch
without changing the source code.

Signed-off-by: Francesco Giudici <[email protected]>
Don't use expired certs in test or CI will fail

Signed-off-by: Francesco Giudici <[email protected]>
@fgiudici
Copy link
Author

A quick drive-by comment: Rather than update formatting to conform to some recent version, it would be cleaner to freeze the Go or gofmt version back to the version at the branching time, to minimize the number of changes on the branch (and indirectly in CRI-O.). (OTOH I haven’t checked whether that’s even possible.)

Good point! I completely agree. I checked, the gofmt is taken from the distro. Also switching back the the "artic" release, the gofmt version will report the unsorted import. I would just skip the docker/tarfile/src.go from the gofmt check. It would be just a Makefile change, we don't want to touch an old and stable release if not really needed.
Pushing the change.

We want something compatible with this old stable branch to run CI
tests. We need also to use F30 for testing (latest Fedora release is
missing package golang-github-cpuguy83-go-md2man... and maybe has some
other incompatibilities we don't need to find)

Signed-off-by: Francesco Giudici <[email protected]>
# Which github repository and branch to use for testing with skopeo
SKOPEO_REPO = containers/skopeo
SKOPEO_BRANCH = master
SKOPEO_BRANCH = 2b97124e
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn’t make much sense to me to use Skopeo commit 2b97124e4ac18c5f56650b89b8710bd5dc450fee (which introduces the two-return-value version of copy.Image) without including the immediately following 2734f93e301c441aabadf1902b7771e3c42bec70 .

The immediately preceding commit might be a better fit.

git clone https://github.com/$(SKOPEO_REPO) $${skopeo_path} && \
cd $${skopeo_path} && git checkout $(SKOPEO_BRANCH) && \
sed -i 's/FROM fedora/FROM fedora:30/g' Dockerfile && \
rm -rf $${vendor_path} && cp -r /gopath/src/github.com/containers/image $${vendor_path} && rm -rf $${vendor_path}/vendor && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anything change about what this line does? If not, wrapping the git checkout in a subshell (cd … && git checkout ) && should allow not changing this line, to make it clearer that nothing is going on, and avoid hard-coding the container layout in this Makefile.

cd $${skopeo_path} && \
git clone https://github.com/$(SKOPEO_REPO) $${skopeo_path} && \
cd $${skopeo_path} && git checkout $(SKOPEO_BRANCH) && \
sed -i 's/FROM fedora/FROM fedora:30/g' Dockerfile && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tests are failing due to

gpg: agent_genkey failed: No pinentry
gpg: key generation failed: No pinentry

And fedora:30 is newer than this code. At least this particular failure can be avoided by going back to :29.

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 30, 2020

A quick drive-by comment: Rather than update formatting to conform to some recent version, it would be cleaner to freeze the Go or gofmt version back to the version at the branching time, to minimize the number of changes on the branch (and indirectly in CRI-O.). (OTOH I haven’t checked whether that’s even possible.)

Good point! I completely agree. I checked, the gofmt is taken from the distro.

It’s actually coming from the Go 1.9.2 tarball extracted in .travis.Dockerfile AFAICS.

Also switching back the the "artic" release, the gofmt version will report the unsorted import.
You’re right about that — actually the breakage comes from a naive backport of 42fb012 , it’s not a version discrepancy as I assumed.

So, please just fix the code to conform; I’m sorry about the hassle.

This pull request was closed.
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.

4 participants