Move to golang 1.18 and later #1522

rhatdan · 2023-02-27T13:56:02Z

Github.com is reporting security issues on older versions of golang.

rhatdan · 2023-02-27T13:58:01Z

@nalind @saschagrunert @flouthoc @giuseppe @vrothberg @umohnani8 PTAL

saschagrunert

LGTM 👍

nalind · 2023-02-27T14:07:12Z

pkg/archive/copy_unix_test.go

@@ -256,8 +256,9 @@ func TestCopyErrDstNotDir(t *testing.T) {
 //

 // A. SRC specifies a file and DST (no trailing path separator) doesn't
-//    exist. This should create a file with the name DST and copy the
-//    contents of the source file into it.
+//


Extra blank line added in the middle of a paragraph. Happens several times below as well, and in meminfo_*.go.

This patch was done with:

$ find . -name *go -exec gofmt -s -w {} ; -print

Followed by make vendor-in-container.

This patch was done with:

$ find . -name *go -exec gofmt -s -w {} ; -print

Okay, that would also modify sources in vendored dependencies, which we don't want to be doing.

Followed by make vendor-in-container.

The target doesn't touch vendored dependencies under tests/tools, which I think is what this warning notification was focusing on. Maybe go get golang.org/x/sys && go mod vendor && go mod tidy from there?

This has not been resolved.

nalind · 2023-02-27T14:07:41Z

This modifies many files under tests/tools/vendor, but not tests/tools/vendor/modules.txt. What's going on there?

rhatdan · 2023-02-27T15:15:10Z

@cevich Any idea on the vendor failure. Is this a matter of updating the golang inside of the VMs?

cevich · 2023-02-27T15:19:15Z

That's strange...the fedora 37 VMs should be on 1.19...looking...

cevich · 2023-02-27T15:27:15Z

...oh! This task is running in a container, that's where the version comes from. In .cirrus.yml:

vendor_task:
    container:
        image: golang
    modules_cache:
        fingerprint_script: cat go.sum
        folder: $GOPATH/pkg/mod
    build_script: make vendor
    test_script: hack/tree_status.sh

That's confusing. It means multiple files need to be updated to reflect a new version in CI. Ugg. Let me see if I can make this task follow the latest Fedora VM instead...

cevich · 2023-02-27T15:44:50Z

...nvm, it seems you know what you're doing.

nalind · 2023-02-28T15:10:36Z

This may also require a bump to the version of the linter installed in tests/tools/Makefile.

rhatdan · 2023-04-01T16:12:14Z

@nalind Any idea what is going wrong now?

nalind · 2023-04-03T13:30:28Z

When I ran it locally, "mount-image" (image-mount.bats:17) failed because btrfs driver does not support mount options (it won't say which one), and "container-on-mounted-image" (image-mount.bats:79) failed because it tried to create an image with the same name as one it has just created (a copy/paste error?).
The other "mount-image" (mount.bats:51) fails because the CLI test helper understands "-r" and "--ro", but not "--read-only" (either the test or the helper should be updated).

rhatdan · 2023-04-03T19:15:32Z

Thanks for pointing to those files, which I have no idea where they came from.

nalind · 2023-04-03T19:18:39Z

pkg/archive/archive_linux_test.go

@@ -21,7 +21,8 @@ import (
 // ├── d2     # opaque, 0750
 // │   └── f1 # empty file, 0660
 // └── d3     # 0700
-//     └── f1 # whiteout, 0000
+//


Extra blank line inserted here for some reason.

GO fmt is doing this I belive.

nalind · 2023-04-03T19:20:17Z

pkg/chunked/cache_linux.go

@@ -398,7 +398,8 @@ func prepareMetadata(manifest []byte) ([]*internal.FileMetadata, error) {
 	toc, err := unmarshalToc(manifest)
 	if err != nil {
 		// ignore errors here.  They might be caused by a different manifest format.
-		return nil, nil
+		logrus.Debugf("could not unmasshal manifest: %v", err)


Typo: "unmasshal" should be "unmarshal".

nalind · 2023-04-03T19:22:16Z

pkg/chunked/compression_linux.go

@@ -149,7 +150,7 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64,
 // readZstdChunkedManifest reads the zstd:chunked manifest from the seekable stream blobStream.  The blob total size must
 // be specified.
 // This function uses the io.github.containers.zstd-chunked. annotations when specified.
-func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, annotations map[string]string) ([]byte, int64, error) {
+func readZstdChunkedManifest(ctx context.Context, blobStream ImageSourceSeekable, blobSize int64, annotations map[string]string) ([]byte, int64, error) {


What's the reason behind adding the context argument here? It doesn't appear to be used.

There was a linter that was complaining about it, so I added it and then had to nolint below. I can remove this, but I think we should start to use more of the linters for storage.

decoder, err := zstd.NewReader(nil) //nolint:contextcheck

The context check wants it passed in here, but I did not know the code well enough to add it. @giuseppe PTAL

nalind · 2023-04-03T19:22:56Z

pkg/parsers/parsers.go

-//     03,1-3      <- this is gonna get parsed as [1,2,3]
-//     3,2,1
-//     0-2,3,1
+//


Extra blank line inserted.

I think go fmt is doing this.

Github.com is reporting security issues on older versions of golang. Signed-off-by: Daniel J Walsh <[email protected]>

giuseppe

LGTM

rhatdan force-pushed the VENDOR branch from 04df15f to 5851305 Compare February 27, 2023 13:56

saschagrunert approved these changes Feb 27, 2023

View reviewed changes

nalind reviewed Feb 27, 2023

View reviewed changes

rhatdan force-pushed the VENDOR branch 2 times, most recently from 3f23a3c to 602f16b Compare February 27, 2023 14:48

rhatdan force-pushed the VENDOR branch from 602f16b to af46d8a Compare February 27, 2023 15:35

rhatdan force-pushed the VENDOR branch from af46d8a to 760e3ad Compare February 28, 2023 12:28

rhatdan force-pushed the VENDOR branch 2 times, most recently from c210f33 to 79904c7 Compare March 5, 2023 14:39

rhatdan force-pushed the VENDOR branch 14 times, most recently from 07174e4 to e9121ae Compare March 31, 2023 17:20

rhatdan force-pushed the VENDOR branch 3 times, most recently from e2a8747 to 0765cc7 Compare April 1, 2023 15:42

rhatdan force-pushed the VENDOR branch from 0765cc7 to 6882e4c Compare April 2, 2023 09:55

rhatdan force-pushed the VENDOR branch from 6882e4c to 12c071a Compare April 3, 2023 18:26

nalind reviewed Apr 3, 2023

View reviewed changes

Move to golang 1.18 and later

a3204cf

Github.com is reporting security issues on older versions of golang. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the VENDOR branch from 12c071a to a3204cf Compare April 3, 2023 19:27

giuseppe approved these changes Apr 4, 2023

View reviewed changes

rhatdan merged commit fab09d7 into containers:main Apr 4, 2023

mtrmac mentioned this pull request Apr 19, 2023

Consider allowing more linters #1579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to golang 1.18 and later #1522

Move to golang 1.18 and later #1522

rhatdan commented Feb 27, 2023

rhatdan commented Feb 27, 2023

saschagrunert left a comment

nalind Feb 27, 2023

rhatdan Feb 27, 2023

nalind Feb 27, 2023

nalind Apr 3, 2023

nalind commented Feb 27, 2023

rhatdan commented Feb 27, 2023

cevich commented Feb 27, 2023

cevich commented Feb 27, 2023 •

edited

Loading

cevich commented Feb 27, 2023

nalind commented Feb 28, 2023

rhatdan commented Apr 1, 2023

nalind commented Apr 3, 2023

rhatdan commented Apr 3, 2023

nalind Apr 3, 2023

rhatdan Apr 3, 2023

nalind Apr 3, 2023

rhatdan Apr 3, 2023

nalind Apr 3, 2023

rhatdan Apr 3, 2023

rhatdan Apr 3, 2023

rhatdan Apr 3, 2023

nalind Apr 3, 2023

rhatdan Apr 3, 2023

giuseppe left a comment

Move to golang 1.18 and later #1522

Move to golang 1.18 and later #1522

Conversation

rhatdan commented Feb 27, 2023

rhatdan commented Feb 27, 2023

saschagrunert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind commented Feb 27, 2023

rhatdan commented Feb 27, 2023

cevich commented Feb 27, 2023

cevich commented Feb 27, 2023 • edited Loading

cevich commented Feb 27, 2023

nalind commented Feb 28, 2023

rhatdan commented Apr 1, 2023

nalind commented Apr 3, 2023

rhatdan commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

cevich commented Feb 27, 2023 •

edited

Loading