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

Bump go-swagger version to 0.30.5 #5162

Merged
merged 2 commits into from
Jul 14, 2023
Merged

Bump go-swagger version to 0.30.5 #5162

merged 2 commits into from
Jul 14, 2023

Conversation

adamkpickering
Copy link
Member

Version 0.28.0 of go-swagger has a bug that calls filepath.EvalSymlinks on a path that is built using the HOME environment variable when the GOPATH environment variable is not set. Since the HOME environment variable is not typically present on Windows, this causes the go-swagger command to fail. Version 0.30.5 solves this problem.

@adamkpickering
Bump go-swagger version to 0.30.5
0bb3205 
Version 0.28.0 of go-swagger has a bug that calls
filepath.EvalSymlinks on a path that is built using the HOME
environment variable when GOPATH is not set. Since the HOME
environment variable is not typically present on Windows, this causes
the go-swagger command to fail. Version 0.30.5 solves this problem.

Signed-off-by: Adam Pickering <adam.pickering@suse.com>
ericpromislow
ericpromislow previously approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Works fine

@adamkpickering adamkpickering mentioned this pull request Jul 13, 2023
Open
@Nino-K
Copy link
Member

Nino-K commented Jul 13, 2023

Just curious if the go.mod file also needs to be updated:

rancher-desktop/src/go/wsl-helper/go.mod

Line 13 in d40c100

github.com/go-swagger/go-swagger v0.28.0

rak-phillip
rak-phillip previously approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

LGTM - I think we hold of on merging with Nino's question pending.

@adamkpickering
Copy link
Member Author

I want to leave a note that bumping the go-swagger dependency in go.mod requires updates to a bunch of other modules because go-swagger requires newer versions of those modules. This means that this PR is larger in scope than I initially thought, so review accordingly.

ericpromislow
ericpromislow requested changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

I found the go unit tests failed until I ran the following commands:

cd src/go/wsl-helper/
go get github.com/rancher-sandbox/rancher-desktop/src/go/wsl-helper/cmd
go get github.com/rancher-sandbox/rancher-desktop/src/go/wsl-helper/pkg/dockerproxy/models
go get -t github.com/rancher-sandbox/rancher-desktop/src/go/wsl-helper/pkg/dockerproxy/util
cd -

git status gives output

On branch bump-go-swagger-version
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   src/go/wsl-helper/go.mod
	modified:   src/go/wsl-helper/go.sum

@jandubois
Copy link
Member

bumping the go-swagger dependency in go.mod requires updates to a bunch of other modules because go-swagger requires newer versions of those modules.

I think it makes sense that we use the same go-swagger version everywhere to avoid future confusion.

@rak-phillip rak-phillip mentioned this pull request Jul 14, 2023
Merged
@adamkpickering
Bump go-swagger to version 0.30.5
5fbf85a 
There are a number of dependencies that are updated
with go-swagger. This is because go-swagger v0.30.5
requires these versions of these dependencies.

Signed-off-by: Adam Pickering <adam.pickering@suse.com>
ericpromislow
ericpromislow approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Works fine, LG

@ericpromislow ericpromislow merged commit 915cf27 into rancher-sandbox:main Jul 14, 2023
rak-phillip
rak-phillip reviewed Jul 14, 2023
View reviewed changes
Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

LGTM - Performed a quick validation by testing install scripts in Windows 11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rak-phillip rak-phillip rak-phillip left review comments

@ericpromislow ericpromislow ericpromislow 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.
5 participants
@adamkpickering @Nino-K @jandubois @ericpromislow @rak-phillip