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

Update Beats to use go 1.11.2 #8746

Merged
merged 4 commits into from
Nov 22, 2018
Merged

Update Beats to use go 1.11.2 #8746

merged 4 commits into from
Nov 22, 2018

Conversation

vjsamuel
Copy link
Contributor

No description provided.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@vjsamuel vjsamuel force-pushed the beats-go11 branch 2 times, most recently from 2580003 to e273658 Compare October 25, 2018 07:09
@@ -39,7 +39,7 @@ func (p *pluginList) String() string {
func (p *pluginList) Set(v string) error {
for _, path := range p.paths {
if path == v {
logp.Warn("%s is already a registered plugin")
logp.Warn("%s is already a registered plugin", pathautodiscover/appenders/config/config.go:69)

Choose a reason for hiding this comment

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

expected selector or type assertion, found 'go'

@vjsamuel vjsamuel force-pushed the beats-go11 branch 4 times, most recently from 7ea5a09 to fbf9151 Compare October 25, 2018 07:29
@@ -20,7 +20,7 @@
package main

import (
"context"
input/log/input.go:513 "context"

Choose a reason for hiding this comment

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

expected 'STRING', found '/' (and 10 more errors)

@vjsamuel vjsamuel force-pushed the beats-go11 branch 8 times, most recently from e015ecf to 90a6568 Compare October 25, 2018 19:32
@ruflin
Copy link
Member

ruflin commented Oct 26, 2018

One change worth mentioning here is that now MacOS 10.10 at least is required:

As announced in the Go 1.10 release notes, Go 1.11 now requires OpenBSD 6.2 or later, macOS 10.10 Yosemite or later, or Windows 7 or later; support for previous versions of these operating systems has been removed.

It's not a problem because our support matrix already has 10.13 in and for APM-server 10.12 : https://www.elastic.co/support/matrix @elastic/apm-server We should probably bring these number in sync.

@elastic/beats Anyone seeing any blockers to jump to 1.11 on master?
@vjsamuel Thanks for tackling this.

@ph
Copy link
Contributor

ph commented Oct 26, 2018

I do not see a blocker to move to 1.11.1, we might see a few hiccups in the tooling but that should not stop us to live on the newest version.

@ph
Copy link
Contributor

ph commented Oct 26, 2018

@vjsamuel I am confused by the commits in that PR, I see a lot of log statement changes?

@graphaelli
Copy link
Member

Thanks @ruflin, I've requested that change to the compat matrix.

@vjsamuel
Copy link
Contributor Author

@ph go 1.11 has enhanced vet to be more strict about correctness of format specifiers:
https://golang.org/doc/go1.11#vet

This caused make check to fail as we do a go vet

@graphaelli
Copy link
Member

This actually uses go 1.11.1 for package builds too - the title of this change should be updated accordingly. Also, we'll need beats-dev/golang-crossbuild:1.11.1* to support that, before this is merged. Finally, all of the FROM golang:1.10.3 in the various Dockerfiles should be updated as well.

Otherwise, +1 to moving to 1.11.1 while it's early in our dev cycle.

@vjsamuel
Copy link
Contributor Author

vjsamuel commented Oct 27, 2018

I have raised elastic/fsevents#4 to fix Auditbeat for OSX. I will update the vendor once it gets mreged.

@vjsamuel vjsamuel force-pushed the beats-go11 branch 2 times, most recently from b6f38d8 to f2821b4 Compare October 27, 2018 02:07
@vjsamuel
Copy link
Contributor Author

@graphaelli i have moved all the containers to 1.11.1. I dont know repo has beats-dev:golang-crossbuild

@ruflin
Copy link
Member

ruflin commented Nov 7, 2018

jenkins, test this

@urso
Copy link

urso commented Nov 7, 2018

Jenkins, please test this.

@ruflin
Copy link
Member

ruflin commented Nov 7, 2018

jenkins, test this

@ruflin
Copy link
Member

ruflin commented Nov 9, 2018

jenkins, test this

1 similar comment
@ruflin
Copy link
Member

ruflin commented Nov 9, 2018

jenkins, test this

@vjsamuel vjsamuel force-pushed the beats-go11 branch 2 times, most recently from ce07966 to f29a883 Compare November 13, 2018 01:05
@urso
Copy link

urso commented Nov 20, 2018

@vjsamuel can you please rebase? There are potential merge conflicts.

I also have had some problems with mage recently when using go1.11.2 (using go1.10.3 did work). Problem was in 6.5 branch, but I'd like to be sure master works.

@urso
Copy link

urso commented Nov 20, 2018

Jenkins, please test this.

@urso
Copy link

urso commented Nov 20, 2018

Jenkins, test this!

@ruflin
Copy link
Member

ruflin commented Nov 21, 2018

jenkins, test this

:go-version: 1.10.3
:release-state: prerelease
:go-version: 1.11.2
:release-state: unreleased
Copy link
Member

Choose a reason for hiding this comment

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

This worries me. Why did this change? This line should be reverted.

Copy link

Choose a reason for hiding this comment

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

I guess rebase did opt for the old one. It must say prerelease

:go-version: 1.10.3
:release-state: prerelease
:go-version: 1.11.2
:release-state: unreleased
Copy link

Choose a reason for hiding this comment

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

I guess rebase did opt for the old one. It must say prerelease

@ruflin
Copy link
Member

ruflin commented Nov 22, 2018

jenkins, test this

1 similar comment
@ruflin
Copy link
Member

ruflin commented Nov 22, 2018

jenkins, test this

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

I've looked at the CI. The windows/Filebeat fails I don't think they are related to this PR it still good to keep our eyes open just in case.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

I've looked at the CI. The windows/Filebeat fails I don't think they are related to this PR it still good to keep our eyes open just in case.

@ruflin ruflin merged commit 8d8eaf3 into elastic:master Nov 22, 2018
@ruflin
Copy link
Member

ruflin commented Nov 22, 2018

@elastic/beats @elastic/apm-server You should probably update your local environment.

@vjsamuel Thanks for keep pushing this one.

DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants