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 version #4059

Closed
ccressent opened this issue Oct 16, 2020 · 2 comments
Closed

Upgrade Go version #4059

ccressent opened this issue Oct 16, 2020 · 2 comments
Assignees
Milestone

Comments

@ccressent
Copy link
Contributor

ccressent commented Oct 16, 2020

We are currently "stuck" with Go 1.13 because it depends on #3729.

Once we are done with the etcd upgrade, updating Go to 1.15 or later will require some work because of the following behaviour:

When running tests with Go 1.15, the following failure is exhibited:

--- FAIL: TestHandlerTypeMutatorField (0.00s)
panic: cannot create context from nil parent
[recovered]
        panic: cannot create context from nil parent
[...]
FAIL    github.com/sensu/sensu-go/backend/apid/graphql  0.074s

This is because of a change introduced in Go 1.15.

Feature Suggestion

Do whatever is necessary do be able to move forward with Go 1.15 and later versions.

Possible Implementation

  1. Fix the failing tests in package backend/api/graphql

I've left a few notes here to achieve that.

  1. Audit the code base to see if we need changes elsewhere

Thinking about the issue, I realized we may need to do much more than just 1. because the issue is not catched at compile time. The Go 1.15 change leads to a panic at runtime. The panic in 1. was catched by tests, but there may be other places that suffer from the same problem, that are not triggered by testing!

Context

I came across failing test cases while running tests on my machine that were passing on CI and other people's machines. We narrowed it down to a change in Go 1.15.

@ccressent
Copy link
Contributor Author

A thought just popped up in my head: I mentioned here that maybe we can alleviate the issue by checking if a context is nil before calling functions like context.WithValue() and replace it with context.Background() if that's the case.

But surely, if it was that simple, that's how the Go team would have gone about fixing it in the context library? Why didn't they do that? Let's be careful with the approach I suggest...

@portertech portertech added this to the 6.2.0 milestone Oct 16, 2020
@palourde palourde removed this from the 6.2.0 milestone Oct 19, 2020
@palourde palourde changed the title Support for Go 1.15 and later Upgrade Go version Oct 20, 2020
@echlebek
Copy link
Contributor

echlebek commented Nov 2, 2020

I think we can analyze sensu-go and our dependencies with https://golangnews.org/2020/07/semgrep-lightweight-static-analysis-for-many-languages/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants