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

chore(ci): remove Go patch version from go.mod #4303

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kaovilai
Copy link

Fixes #4292

@jkowalski
Copy link
Contributor

@julio-lopez do you see any reasons we should not do it?

@jkowalski jkowalski changed the title Remove Go patch version from go.mod chore(ci): remove Go patch version from go.mod Dec 14, 2024
@jkowalski
Copy link
Contributor

@kaovilai looks like it's not compiling.

@kaovilai
Copy link
Author

Will address. Thanks.

@kaovilai kaovilai force-pushed the patch-1 branch 2 times, most recently from cde3754 to 9453235 Compare December 14, 2024 08:23
@kaovilai
Copy link
Author

❯ make build-current-os-noui 
go build -ldflags "-s -w -X github.com/kopia/kopia/repo.BuildVersion=20241214.0.0-94532351 -X github.com/kopia/kopia/repo.BuildInfo=94532351b6567678323f6ac02096d9b119402843 -X github.com/kopia/kopia/repo.BuildGitHubRepo=" -o dist/kopia_darwin_universal/kopia github.com/kopia/kopia

~/git/kopia patch-1
❯ go vet ./...

works now.

Copy link

codecov bot commented Dec 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.00%. Comparing base (cb455c6) to head (23a7fa4).
Report is 371 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4303      +/-   ##
==========================================
+ Coverage   75.86%   76.00%   +0.13%     
==========================================
  Files         470      508      +38     
  Lines       37301    38963    +1662     
==========================================
+ Hits        28299    29612    +1313     
- Misses       7071     7382     +311     
- Partials     1931     1969      +38     

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

Comment on lines 44 to 47
go-version-file: 'go.mod'
# go-version-file do not account for latest compatible go https://github.com/actions/setup-go/issues/481
# which will break https://github.com/kopia/kopia/issues/4292 requirement if used
# leave below commented for vuln check to ignore vulns fixed in latest go
# go-version-file: 'go.mod'
go-version-input: '1.22'
Copy link
Author

Choose a reason for hiding this comment

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

.github/workflows/code-coverage.yml Outdated Show resolved Hide resolved
@julio-lopez
Copy link
Collaborator

@julio-lopez do you see any reasons we should not do it?

See inline comment.

@kaovilai kaovilai force-pushed the patch-1 branch 4 times, most recently from 6c808a9 to d6cf363 Compare December 17, 2024 06:28
@kaovilai kaovilai requested a review from julio-lopez December 17, 2024 06:30
@@ -1,6 +1,6 @@
module github.com/kopia/kopia

go 1.22.7
go 1.22
Copy link
Author

@kaovilai kaovilai Dec 17, 2024

Choose a reason for hiding this comment

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

@@ -59,7 +59,7 @@ require (
go.uber.org/zap v1.27.0
golang.org/x/crypto v0.30.0
golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1
golang.org/x/mod v0.22.0
golang.org/x/mod v0.20.1-0.20240815161730-b1d336cfca97 // this version is equivalent to 0.22.0 except it excludes a commit that enforces go directive patch version be used https://github.com/golang/mod/commit/3afcd4e90a74c23515a9543f1e8fb68f05ecc8e0 We would want to remove this when 0.23.0 is released
Copy link
Author

Choose a reason for hiding this comment

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

@julio-lopez Is this ok?

Copy link
Author

Choose a reason for hiding this comment

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

on Go versions 1.21.0 up to 1.21.10,
the toolchain upgrade logic will try to download the release "1.22",
which doesn't exist.

The patch difference with 0.22.0 do not applies beneficially to this repo which is already building on 1.22+ go binaries.

Copy link
Author

Choose a reason for hiding this comment

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

I would expect by 1.23.0 go if golang/mod upgrades to it that they would drop 1.22.0 to 1.23 instead of 1.23.0

Copy link
Author

Choose a reason for hiding this comment

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

long term it might not make sense to use this module, it's only used for the semver package at the moment.

This module states in its readme

packages for writing tools that work directly with Go module mechanics

Copy link
Author

@kaovilai kaovilai Dec 18, 2024

Choose a reason for hiding this comment

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

I've been told the alternative to excluding this is to set toolchain directive to 1.22.10 which should not affect imported modules.
go directive can remain at 1.22.0.

Copy link
Author

Choose a reason for hiding this comment

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

I have additionally requested changes at #4248 (review) to satisfy the same requirement.

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.

deps: Remove Go patch version from go.mods
3 participants