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

upgrade Go to 1.20.6 #36000

Merged
merged 16 commits into from
Jul 28, 2023
Merged

upgrade Go to 1.20.6 #36000

merged 16 commits into from
Jul 28, 2023

Conversation

AndersonQ
Copy link
Member

@AndersonQ AndersonQ commented Jul 5, 2023

What does this PR do?

Why is it important?

  • To keep up with the latest Go improvements, features and bug fixes.
  • Test coverage is broken due to the usage of //line directives on generated code which makes go tool cover to look for files in the wrong path, failing to generate the coverage report.

Checklist

  • [ ] My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@AndersonQ AndersonQ added enhancement Team:Elastic-Agent Label for the Agent team backport-skip Skip notification from the automated backport with mergify 8.10-candidate labels Jul 5, 2023
@AndersonQ AndersonQ self-assigned this Jul 5, 2023
@AndersonQ AndersonQ requested review from a team as code owners July 5, 2023 15:07
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 5, 2023
@AndersonQ AndersonQ removed the Team:Elastic-Agent Label for the Agent team label Jul 5, 2023
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 5, 2023
@AndersonQ AndersonQ added Team:Elastic-Agent Label for the Agent team and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 5, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this deleted?

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 5, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-07-28T04:58:16.662+0000

  • Duration: 146 min 19 sec

Test stats 🧪

Test Results
Failed 0
Passed 27203
Skipped 2001
Total 29204

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@cmacknz
Copy link
Member

cmacknz commented Jul 11, 2023

I suspect we'll want the changes in elastic/elastic-agent@db1c47f here as well since the code is the same.

I also suspect the unit tests in the dev-tool directory aren't being run by CI, because those tests are failing with Go 1.20.5 locally on main.

@cmacknz
Copy link
Member

cmacknz commented Jul 24, 2023

[2023-07-24T16:44:49.500Z] cover: can't read "github.com/elastic/beats/v7/filebeat/input/syslog/format_check.rl": open /var/lib/jenkins/workspace/PR-36000-8-47b742ad-51f3-47c4-8aec-0c151ac4fbea/src/github.com/elastic/beats/filebeat/input/syslog/format_check.rl: no such file or directory

[2023-07-24T16:44:49.500Z] Error: failed to write HTML code coverage report: running "go tool cover -html=build/TEST-go-unit.cov -o build/TEST-go-unit.html" failed with exit code 1

script returned exit code 1

It looks like this problem might have existed before this PR but now the error is actually being caught: #35927. This plausibly could have been a bug fix in go tool cover as part of supporting code coverage for integration tests.

Not sure how to fix it yet, possibly we need to avoid generating code coverage for the package containing these generated files.

the //line directives used to point to the original "ragel" files were creating problems for the coverage tool. They were being reported without the 'parser/' directory. Causing the go tool cover to look for the wrong files and therefore failing.
Moving it all to the root directory fixes the problem.
@cmacknz
Copy link
Member

cmacknz commented Jul 25, 2023

Go 1.20.6 was just released, you can bump the Go versions here to 1.20.6 to save from having to follow up with another PR doing that.

@AndersonQ
Copy link
Member Author

/test

@mergify
Copy link
Contributor

mergify bot commented Jul 26, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b upgrade-go upstream/upgrade-go
git merge upstream/main
git push upstream upgrade-go

@AndersonQ AndersonQ changed the title upgrade Go to 1.20.5 upgrade Go to 1.20.6 Jul 26, 2023
@AndersonQ
Copy link
Member Author

/test

@AndersonQ AndersonQ enabled auto-merge (squash) July 27, 2023 12:59
@@ -156,7 +156,7 @@ func TestParseRFC3164(t *testing.T) {
hostname: "test-host",
msg: "this is the message",
},
wantErr: `validation error at position 5: parsing time "24-08-2003T05:14:15-07:00" as "2006-01-02T15:04:05.999999999Z07:00": cannot parse "8-2003T05:14:15-07:00" as "2006"`,
wantErr: `validation error at position 5: parsing time "24-08-2003T05:14:15-07:00" as "2006-01-02T15:04:05.999999999Z07:00": cannot parse "24-08-2003T05:14:15-07:00" as "2006"`,
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change? Are you sure this is a not a functional change in the parser that caused this? Was there a change in Go 1.20 that needed this?

I want to make sure this is not a hint that something has changed in how we parse values that we need to account for before we release this.

@mergify
Copy link
Contributor

mergify bot commented Jul 27, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b upgrade-go upstream/upgrade-go
git merge upstream/main
git push upstream upgrade-go

@pierrehilbert pierrehilbert disabled auto-merge July 28, 2023 08:49
@pierrehilbert pierrehilbert merged commit a278734 into elastic:main Jul 28, 2023
1 check passed
@AndersonQ AndersonQ deleted the upgrade-go branch July 28, 2023 14:08
@cmacknz cmacknz added the backport-7.17 Automated backport to the 7.17 branch with mergify label Oct 16, 2023
mergify bot pushed a commit that referenced this pull request Oct 16, 2023
* upgrade Go to 1.20.5

* ignore known issue with go tool cover

* capture cover output and check it for errors

* debug

* wip

* move ragel files to root directory

the //line directives used to point to the original "ragel" files were creating problems for the coverage tool. They were being reported without the 'parser/' directory. Causing the go tool cover to look for the wrong files and therefore failing.
Moving it all to the root directory fixes the problem.

* upgrade to go1.20.6

* .

* Revert "move ragel files to root directory"

This reverts commit 2b6a159.

* remove debug

* disable //line directives

* goimport

(cherry picked from commit a278734)

# Conflicts:
#	.go-version
#	.golangci.yml
#	Vagrantfile
#	auditbeat/Dockerfile
#	dev-tools/kubernetes/filebeat/Dockerfile.debug
#	dev-tools/kubernetes/heartbeat/Dockerfile.debug
#	dev-tools/kubernetes/metricbeat/Dockerfile.debug
#	dev-tools/mage/gotest.go
#	filebeat/input/syslog/rfc5424_parser.go
#	heartbeat/Dockerfile
#	libbeat/docs/version.asciidoc
#	libbeat/reader/syslog/rfc3164_test.go
#	libbeat/reader/syslog/rfc5424_test.go
#	metricbeat/Dockerfile
#	metricbeat/module/http/_meta/Dockerfile
#	metricbeat/module/nats/_meta/Dockerfile
#	packetbeat/Dockerfile
#	x-pack/functionbeat/Dockerfile
#	x-pack/metricbeat/module/stan/_meta/Dockerfile
cmacknz added a commit that referenced this pull request Oct 24, 2023
* upgrade Go to 1.20.6 (#36000)

* upgrade Go to 1.20.5

* ignore known issue with go tool cover

* capture cover output and check it for errors

* debug

* wip

* move ragel files to root directory

the //line directives used to point to the original "ragel" files were creating problems for the coverage tool. They were being reported without the 'parser/' directory. Causing the go tool cover to look for the wrong files and therefore failing.
Moving it all to the root directory fixes the problem.

* upgrade to go1.20.6

* .

* Revert "move ragel files to root directory"

This reverts commit 2b6a159.

* remove debug

* disable //line directives

* goimport

(cherry picked from commit a278734)

# Conflicts:
#	.go-version
#	.golangci.yml
#	Vagrantfile
#	auditbeat/Dockerfile
#	dev-tools/kubernetes/filebeat/Dockerfile.debug
#	dev-tools/kubernetes/heartbeat/Dockerfile.debug
#	dev-tools/kubernetes/metricbeat/Dockerfile.debug
#	dev-tools/mage/gotest.go
#	filebeat/input/syslog/rfc5424_parser.go
#	heartbeat/Dockerfile
#	libbeat/docs/version.asciidoc
#	libbeat/reader/syslog/rfc3164_test.go
#	libbeat/reader/syslog/rfc5424_test.go
#	metricbeat/Dockerfile
#	metricbeat/module/http/_meta/Dockerfile
#	metricbeat/module/nats/_meta/Dockerfile
#	packetbeat/Dockerfile
#	x-pack/functionbeat/Dockerfile
#	x-pack/metricbeat/module/stan/_meta/Dockerfile

* Resolve conflicts.

* Remove extra entries in CHANGELOG.next.asciidoc

* Remove extra files.

* Fix test coverage report in cef parser

* Run mage fmt

---------

Co-authored-by: Anderson Queiroz <[email protected]>
Co-authored-by: Craig MacKenzie <[email protected]>
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
* upgrade Go to 1.20.5

* ignore known issue with go tool cover

* capture cover output and check it for errors

* debug

* wip

* move ragel files to root directory

the //line directives used to point to the original "ragel" files were creating problems for the coverage tool. They were being reported without the 'parser/' directory. Causing the go tool cover to look for the wrong files and therefore failing.
Moving it all to the root directory fixes the problem.

* upgrade to go1.20.6

* .

* Revert "move ragel files to root directory"

This reverts commit 2b6a159.

* remove debug

* disable //line directives

* goimport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.10-candidate backport-7.17 Automated backport to the 7.17 branch with mergify backport-skip Skip notification from the automated backport with mergify enhancement Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix coverage report.
7 participants