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

fix: use the right Go version in the CI #346

Merged
merged 5 commits into from
May 13, 2021
Merged

fix: use the right Go version in the CI #346

merged 5 commits into from
May 13, 2021

Conversation

mdelapenya
Copy link
Contributor

@mdelapenya mdelapenya commented May 13, 2021 •
edited
Loading

What does this PR do?

This PR sets an environment variable on CI, reading this project's .go-version file and setting that value to GO_VERSION. Then, because the withGoEnv built-in step supports passing a version, we are using the new variable as an input parameter in the step. As a consequence, the step will internally use GVM to setup the Go environment for that version.

The PR also replaces Gimme with gvm, and finally uses the existing script to set the Go version in the execution of the tests.

Why is it important?

We want to run the code in the proper version

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

Follow-ups

I'd recommend moving to gvm in the automation scripts, to match what the CI does. I did that in f888d11, but we can revert that commit if you disagree.

@mdelapenya mdelapenya self-assigned this May 13, 2021
@mdelapenya mdelapenya requested review from a team, michel-laterman and blakerouse May 13, 2021 09:03
@mdelapenya
Copy link
Contributor Author

The 1.16.2 version (the one in the .go-version file) is honoured.

Screenshot 2021-05-13 at 11 07 36

@elasticmachine
Copy link
Contributor

elasticmachine commented May 13, 2021 •
edited
Loading

💚 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #346 updated

  • Start Time: 2021-05-13T13:45:54.491+0000

  • Duration: 5 min 13 sec

  • Commit: 21328bf

Test stats 🧪

Test Results
Failed 0
Passed 174
Skipped 0
Total 174

Trends 🧪

Image of Build Times

Image of Tests

kuisathaverat
kuisathaverat reviewed May 13, 2021
View reviewed changes
.ci/Jenkinsfile Outdated
}
}
stage('Check') {
options { skipDefaultCheckout() }
steps {
cleanup()
withGoEnv(){
withGoEnv(version: "${GO_VERSION}"){
Copy link

Choose a reason for hiding this comment

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

withGoEnv() will search for the GO_VERSION or the file .go-version because it uses goDefaultVersion()

https://github.com/elastic/apm-pipeline-library/blob/master/vars/withGoEnvUnix.groovy#L37
https://github.com/elastic/apm-pipeline-library/blob/master/vars/goDefaultVersion.groovy#L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We must be missing something then, as previous executions are not setting the GO_VERSION properly: https://beats-ci.elastic.co/blue/organizations/jenkins/Ingest-manager%2Ffleet-server/detail/PR-289/12/pipeline/143. As you can see there, the version is 1.15.8, even though the .go-version file for that PR is set to 1.16.2 (https://github.com/elastic/fleet-server/blob/60c9b4647d913e018471fb790629a27f65097786/.go-version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kuisathaverat after reading the code in the pipeline step, I'm pretty sure this line https://github.com/elastic/apm-pipeline-library/blob/master/vars/goDefaultVersion.groovy#L29 is evaluated to false, therefore the goDefaultVersion is not overridden and it takes the default value: 1.15.8 (https://github.com/elastic/apm-pipeline-library/blob/master/vars/goDefaultVersion.groovy#L41)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, we are reading the file from:

  • this PR: readFile("${env.WORKSPACE}/${env.BASE_DIR}/.go-version")
  • in the step: readFile(file: ".go-version")?.trim()

So I think the step should always read from BASE_DIR, as it's a common pattern we use. Do you think we should open a bug in the library?

Copy link

@kuisathaverat kuisathaverat May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we should run withGoEnv() at root source level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh good catch! Will send a commit with that change, thanks!

kuisathaverat
kuisathaverat reviewed May 13, 2021
View reviewed changes
.ci/Jenkinsfile Outdated Show resolved Hide resolved
kuisathaverat
kuisathaverat reviewed May 13, 2021
View reviewed changes
.ci/Jenkinsfile Outdated Show resolved Hide resolved
@mdelapenya
fix: use WithGoEnv properly
21328bf 
If we call it within the BASE_DIR, it will automatically infer the .go-version
reading it from the root dir
kuisathaverat
kuisathaverat reviewed May 13, 2021
View reviewed changes
.ci/Jenkinsfile Outdated Show resolved Hide resolved
kuisathaverat
kuisathaverat reviewed May 13, 2021
View reviewed changes
.ci/Jenkinsfile Outdated Show resolved Hide resolved
@mdelapenya mdelapenya marked this pull request as ready for review May 13, 2021 13:47
@mdelapenya mdelapenya mentioned this pull request May 13, 2021
Merged
michel-laterman
michel-laterman approved these changes May 13, 2021
View reviewed changes
Copy link
Contributor

@michel-laterman michel-laterman left a comment

Choose a reason for hiding this comment

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

thanks for fixing this!

@mdelapenya mdelapenya added the Team:Automation Label for the Observability productivity team label May 13, 2021
@mdelapenya mdelapenya merged commit e253a18 into elastic:master May 13, 2021
@mdelapenya mdelapenya deleted the go-version branch May 14, 2021 05:32
@mdelapenya mdelapenya mentioned this pull request May 14, 2021
Merged
6 tasks
@mergify mergify bot mentioned this pull request Jun 15, 2021
Merged
@scunningham scunningham mentioned this pull request Jun 15, 2021
Merged
mergify bot added a commit that referenced this pull request Jun 15, 2021
@mergify @mdelapenya
[7.x](backport #346) fix: use the right Go version in the CI (#460)
6af3e35 
* fix: use the right Go version in the CI

(cherry picked from commit 40e6fb3)

* chore: replace gimme with gvm

(cherry picked from commit f888d11)

* chore: enforce running the tests with go version

(cherry picked from commit 078b9bc)

# Conflicts:
#	Makefile

* Revert "fix: use the right Go version in the CI"

This reverts commit 40e6fb3.

(cherry picked from commit 0871589)

* fix: use WithGoEnv properly

If we call it within the BASE_DIR, it will automatically infer the .go-version
reading it from the root dir

(cherry picked from commit 21328bf)

* Update Makefile

Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Sean Cunningham <[email protected]>
@v1v v1v added the v7.13.0 label Jul 8, 2021
This was referenced Jul 8, 2021
Closed
Merged
mergify bot added a commit that referenced this pull request Jul 8, 2021
@mergify @mdelapenya
[7.13](backport #346) fix: use the right Go version in the CI (#532)
0391c63 
* fix: use the right Go version in the CI

(cherry picked from commit 40e6fb3)

* chore: replace gimme with gvm

(cherry picked from commit f888d11)

* chore: enforce running the tests with go version

(cherry picked from commit 078b9bc)

* Revert "fix: use the right Go version in the CI"

This reverts commit 40e6fb3.

(cherry picked from commit 0871589)

* fix: use WithGoEnv properly

If we call it within the BASE_DIR, it will automatically infer the .go-version
reading it from the root dir

(cherry picked from commit 21328bf)

Co-authored-by: Manuel de la Peña <[email protected]>
@v1v
Copy link
Member

v1v commented Jul 8, 2021

@Mergifyio backport 7.14

@mergify mergify bot mentioned this pull request Jul 8, 2021
Closed
@mergify
Copy link
Contributor

mergify bot commented Jul 8, 2021

Command backport 7.14: success

Backports have been created

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

@michel-laterman michel-laterman michel-laterman approved these changes

@kuisathaverat kuisathaverat kuisathaverat approved these changes

@v1v v1v v1v approved these changes

@blakerouse blakerouse Awaiting requested review from blakerouse

Assignees

@mdelapenya mdelapenya

Labels
Team:Automation Label for the Observability productivity team v7.13.0 v7.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Go version is not properly set on CI
6 participants
@mdelapenya @elasticmachine @v1v @kuisathaverat @michel-laterman @scunningham