Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

[Build] Support go 1.17 #381

Merged
merged 1 commit into from
Aug 24, 2021
Merged

[Build] Support go 1.17 #381

merged 1 commit into from
Aug 24, 2021

Conversation

roopakv
Copy link
Contributor

@roopakv roopakv commented Aug 22, 2021

Summary

Updates mmctl to support go1.17 as described here: https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md

This is currently blocking homebrew here: Homebrew/homebrew-core#83413

Ticket Link

#363

@mattermod
Copy link
Contributor

Hello @roopakv,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

@roopakv
Copy link
Contributor Author

roopakv commented Aug 22, 2021

/check-cla

@codecov-commenter
Copy link

Codecov Report

Merging #381 (e0bcd81) into master (41b3f6a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #381   +/-   ##
=======================================
  Coverage   67.99%   67.99%           
=======================================
  Files          44       44           
  Lines        4490     4490           
=======================================
  Hits         3053     3053           
  Misses       1243     1243           
  Partials      194      194           
Impacted Files Coverage Δ
commands/utils_unix.go 41.17% <ø> (ø)

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 41b3f6a...e0bcd81. Read the comment docs.

@roopakv
Copy link
Contributor Author

roopakv commented Aug 23, 2021

@streamer45 would you be able to take a look at this please.

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Aug 23, 2021
@streamer45 streamer45 self-assigned this Aug 23, 2021
Copy link
Contributor

@streamer45 streamer45 left a comment

Choose a reason for hiding this comment

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

Thank you @roopakv 👍

@streamer45 streamer45 linked an issue Aug 23, 2021 that may be closed by this pull request
@roopakv
Copy link
Contributor Author

roopakv commented Aug 23, 2021

awesome. @streamer45 would you also be able to cut a new release after merging this :) 🙏

@streamer45
Copy link
Contributor

awesome. @streamer45 would you also be able to cut a new release after merging this :)

Let me ping someone from our release team and see what we can do about that. /cc @metanerd

@metanerd
Copy link
Contributor

Sure thing! I guess releases 5.36 and up are affected by homebrew?

@roopakv
Copy link
Contributor Author

roopakv commented Aug 23, 2021

@metanerd if you look at Homebrew/homebrew-core#83413, we are unable to update to go 1.17 on homebrew because right now mmctl is not compatible. So after merging this PR if you could cut a new release that would unblock brew

@metanerd
Copy link
Contributor

@roopakv Sorry, I have seen this, but I am not sure which release version of mmctl homebrew pulls in. Would you be able to provide me further information please? I can unfortunately not check this on my desktop.

@roopakv
Copy link
Contributor Author

roopakv commented Aug 23, 2021

@metanerd right now we pull in 5.38 see here

Once you release a new version we will update to that

@metanerd
Copy link
Contributor

@roopakv Ah, I see. Awesome, thank you! I can provide a Patch release after merge.

@roopakv
Copy link
Contributor Author

roopakv commented Aug 23, 2021

@metanerd @streamer45 anything i need to do before we can merge? :)

@metanerd
Copy link
Contributor

Staying safe and taking care! ;)
Agniva will get to it soon ™️

@agnivade
Copy link
Member

Thank you for the PR @roopakv! I guess the problem here is that brew by itself is also doing a gofmt, which is causing the new lines to be added. Go 1.17 is of course fully backwards compatible with the old syntax.

I am wondering if it would be possible for brew to fetch and build from master for the time being until we cut a new release? I am not sure if cutting a new release is justified because the gofmt command from a later release generates some new lines.

This PR is of course good to be merged.

@roopakv
Copy link
Contributor Author

roopakv commented Aug 24, 2021

@agnivade brew has to fetch from a release never master. would really appreciate unblocking go 1.17 for the whole community rather than having to wait for just mmctl.

you can read more about why brew moves the community forward rather than pin different versions for just a single formuale on the issue linked above.

@agnivade can you also merge the PR? I don't have that ability to do so myself

@agnivade agnivade merged commit 9dc637d into mattermost:master Aug 24, 2021
@roopakv roopakv deleted the roopak/gobbuild branch August 24, 2021 04:51
@agnivade
Copy link
Member

Thanks @roopakv. Building from 1.17 is fine, and forcing the community to always be on the latest is also okay. But 1.17 was released explicitly on the premise that older build tags will still work. The real issue here is brew is blocked on the gofmt errors which I believe should should not be something that brew should enforce.

Making a patch release to fix a cosmetic issue changes the semantics of a patch release.

That being said, I would defer to @amyblais and @metanerd on this matter.

@roopakv
Copy link
Contributor Author

roopakv commented Aug 24, 2021

@agnivade that makes sense and I will work with the brew folks to avoid running gofmt on builds. If you look at the other packages having issues they were forced to upgrade x/sys because of a crash with go 1.17 :)

However right now I urge mmctl to make an exception rules and cut a release :)

metanerd added a commit that referenced this pull request Aug 24, 2021
metanerd added a commit that referenced this pull request Aug 24, 2021
* Build on 1.17.0 for #381
@amyblais
Copy link
Member

Is there anything specific needed from me for this? Does it mean this would get included in a v5.38.2 dot release?

metanerd pushed a commit that referenced this pull request Aug 24, 2021
metanerd added a commit that referenced this pull request Aug 24, 2021
* Build on 1.17.0 for #381
@metanerd
Copy link
Contributor

AFAIK nothing specific needed. It will be auto included in the v5.38.2 dot release. I will change the version, so it gets included in server.

metanerd added a commit that referenced this pull request Aug 24, 2021
* Lock CI from /release-5.38 (#369)

Co-authored-by: Mmbot <mmbot@mattermost>

* [Build] Support go 1.17 (#381)

* Build on 1.17.0 for #381 (#383)

* Build on 1.17.0 for #381

* Use consistent image (#384)

* Use consistent and newer images.

* Bump cache key

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Co-authored-by: Mmbot <mmbot@mattermost>
Co-authored-by: Roopak Venkatakrishnan <roopak.v@gmail.com>
@metanerd
Copy link
Contributor

@roopakv Thank you for your patience! Would be ready to bump this now: https://github.com/Homebrew/homebrew-core/blob/0c0565210aefb74878d87a8af3736680ea7da6a8/Formula/mmctl.rb#L5

https://github.com/mattermost/mmctl/releases/tag/v5.38.1

metanerd added a commit that referenced this pull request Aug 26, 2021
* [Build] Support go 1.17 (#381)

* Build on 1.17.0 for #381 (#383)

* Build on 1.17.0 for #381

* Use consistent image (#384)

* Use consistent and newer images.

* Bump cache key

Co-authored-by: Roopak Venkatakrishnan <roopak.v@gmail.com>
metanerd added a commit that referenced this pull request Aug 26, 2021
* [Build] Support go 1.17 (#381)

* Build on 1.17.0 for #381 (#383)

* Build on 1.17.0 for #381

* Use consistent image (#384)

* Use consistent and newer images.

* Bump cache key

Co-authored-by: Roopak Venkatakrishnan <roopak.v@gmail.com>
@metanerd
Copy link
Contributor

I am sorry @roopakv , it seems the changes were not applied for v5.38.1. But are now definitely in v5.38.2 now. You can however also already go with v5.39.1
Sorry for any inconveniences this caused.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gofmt error in v5.36.0 during brew build
7 participants