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

kind/feature: Bump build to Go 1.17 #164

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

stoovon
Copy link
Contributor

@stoovon stoovon commented Oct 23, 2021

What this PR does / why we need it:

Update Go version to 1.17

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #159

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@yitsushi
Copy link
Contributor

Can you update github actions too?

@jmickey jmickey added area/dependency Issues or PRs related to dependency changes kind/cleanup Removing things previously overlooked labels Oct 25, 2021
@jmickey jmickey requested a review from a team October 25, 2021 18:13
@jmickey jmickey assigned jmickey and unassigned jmickey Oct 25, 2021
@richardcase
Copy link
Member

@stoovon - thanks for the PR. Is there any chance you could rebase and also update the GitHub actions to use 1.17?

@richardcase richardcase added kind/refactor Only refactoring changes and removed kind/cleanup Removing things previously overlooked labels Oct 27, 2021
@stoovon
Copy link
Contributor Author

stoovon commented Oct 27, 2021 via email

@richardcase
Copy link
Member

Great, thanks @stoovon 👍 Hope the conference was good.

@stoovon
Copy link
Contributor Author

stoovon commented Oct 27, 2021 via email

@stoovon
Copy link
Contributor Author

stoovon commented Oct 27, 2021

@richardcase, @yitsushi updated Github workflows

Copy link
Contributor

@yitsushi yitsushi left a comment

Choose a reason for hiding this comment

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

image

I was very curious what happened... 😆

You committed your vendor directory (which we don't have, therefore it's not in .gitignore)

@stoovon
Copy link
Contributor Author

stoovon commented Oct 27, 2021

image

I was very curious what happened... laughing

You committed your vendor directory (which we don't have, therefore it's not in .gitignore)

My apologies. Removed.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2021

Codecov Report

Merging #164 (840242c) into main (94ab549) will increase coverage by 7.40%.
The diff coverage is 55.55%.

❗ Current head 840242c differs from pull request most recent head 338ae84. Consider uploading reports for the commit 338ae84 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   47.00%   54.40%   +7.40%     
==========================================
  Files          45       45              
  Lines        1951     1963      +12     
==========================================
+ Hits          917     1068     +151     
+ Misses        930      788     -142     
- Partials      104      107       +3     
Impacted Files Coverage Δ
infrastructure/firecracker/config.go 0.00% <0.00%> (ø)
core/steps/runtime/initrd_mount.go 100.00% <100.00%> (+100.00%) ⬆️
core/steps/runtime/kernel_mount.go 100.00% <100.00%> (+100.00%) ⬆️
core/steps/runtime/volume_mount.go 100.00% <100.00%> (+100.00%) ⬆️
infrastructure/containerd/event_service.go 59.64% <0.00%> (+3.50%) ⬆️
core/steps/runtime/dir_create.go 73.07% <0.00%> (+30.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28138da...338ae84. Read the comment docs.

@stoovon
Copy link
Contributor Author

stoovon commented Oct 27, 2021

Also squashed the bad commit.

@jmickey
Copy link
Contributor

jmickey commented Oct 28, 2021

@stoovon Thanks for fixing that. It seems something went a little weird with the go.mod files in both the root and hack/tools directory. Might need to run go mod tidy.

@stoovon
Copy link
Contributor Author

stoovon commented Oct 28, 2021

Thanks @jmickey. I removed the indirect sections, moved e.g. https://github.com/golang/go/issues/27309 // require to the first section, re-ran go mod tidy and go mod vendor and ended up in the same place.

The new format for mod files (separate indirect dependencies section) is, as I understand it, new for go 1.17.x and was introduced (or at least worked on) as part of golang/go#45965. So I think this is working as intended.

I also checked a number of other go 1.17 projects and I think this is the default way of doing things now (appreciate it's a significant change from the old way of not listing indirect dependencies which is why the diff looks strange - apologies for not explaining this in a comment on my commit).

I couldn't see a way to avoid the separate indirect dependencies section; when I merge them together go mod tidy splits them out again.

@bigkevmcd
Copy link

@stoovon Yes, this was covered in the 1.17 release notes https://golang.org/doc/go1.17#go-command

@yitsushi
Copy link
Contributor

I like the idea of listing and pinning all indirect dependencies.

yitsushi
yitsushi previously approved these changes Oct 28, 2021
@jmickey
Copy link
Contributor

jmickey commented Oct 28, 2021

@stoovon Ah right, sorry I should have read the go1.17 release notes, sorry about that.

@stoovon
Copy link
Contributor Author

stoovon commented Oct 28, 2021

Thanks for the approval @yitsushi. Pushed verified commits now so should be mergeable.

@yitsushi yitsushi merged commit 4b5af4c into liquidmetal-dev:main Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependency Issues or PRs related to dependency changes kind/refactor Only refactoring changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump to go 1.17
6 participants