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

chore: upgrade to 1.21 #794

Merged
merged 4 commits into from
May 15, 2024
Merged

chore: upgrade to 1.21 #794

merged 4 commits into from
May 15, 2024

Conversation

lzap
Copy link
Member

@lzap lzap commented May 9, 2024

Bump to Go Toolbox 1.21.6 from RHEL 9.4.

Signed-off-by: Lukas Zapletal <[email protected]>
@lzap
Copy link
Member Author

lzap commented May 9, 2024

Omg I knew this is gonna hit us some day. And it hits hard. Go 1.21 updated how init() functions are called. Unfortunately, we heavily rely on init functions for stub clients, job queue and other stuff instead calling explicit initializers. It is more convenient for ability to run tests individually. However, ordering is a mess:

$ GODEBUG=inittrace=1 make test 2>&1| grep init | grep provisioning

Unfortunately, go test does not work well with inittrace. It can be exported but then it is a mess.

@ezr-ondrej
Copy link
Member

I need to figure out how to solve this.

Good topic for a tech discussion I'd say :)

@lzap
Copy link
Member Author

lzap commented May 10, 2024

I have half of the patch done, will finish it on Monday. I got rid of all init functions, but it is quite big one.

@lzap
Copy link
Member Author

lzap commented May 13, 2024

So I was working for two days on complete refactoring of init trying to make it explicit, but I ran into way too many import cycle problems that manifested only after side effect importing was done for some reason. I am falling back now to figure out a different way out.

@lzap lzap force-pushed the bump-go-1-21 branch 2 times, most recently from b72166f to 5474077 Compare May 13, 2024 16:44
@@ -9,7 +9,7 @@ import (
"github.com/RHEnVision/provisioning-backend/internal/ptr"
)

var ReservationTime time.Time = MustParseTime("2013-05-13T19:20:25Z")
var ReservationTime = MustParseTime("2013-05-13T19:20:25Z")
Copy link
Member Author

Choose a reason for hiding this comment

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

There are couple of changes like this, I did not pay attention and IDE fixed these for me. These are not relevant, will not hurt I guess.

Copy link
Member

Choose a reason for hiding this comment

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

I like it, that's the way C++ headed last time I lookes as well, no useless explicity :)

@lzap
Copy link
Member Author

lzap commented May 13, 2024

So the solution was rather simple, but I spent quite some time trying to do a big-bang refactoring, I ran into many import cycle problems and it was a dead end.

The solution I am proposing is to move all GetXXX function init() initializers into files named 0init.go and include or exclude them depending on if unit tests are executed. Because it is not possible to guarantee the order of when package is being imported it sometimes loaded stubs and then overwritten the GetXXX function with real implementation later on. I tried to only set once, but that did not do the trick so the final solution is to actually avoid this init() code alltogether. When running real code (no build tags) or integration tests then only the relevant init() function will be called.

Lessons learned:

* Never ever use init() this way anymore, it is fine for local initialization but this is a pain to debug or work with.

  • Think more about inter-package dependencies to avoid import cycles. Specifically the design when we have a common package with many common code and types (e.g. dao) and then packages which use that (e.g. dao/pxy and dao/stub) are prone to import cycles.

@lzap
Copy link
Member Author

lzap commented May 14, 2024

/retest

2 similar comments
@adarshdubey-star
Copy link
Member

/retest

@adarshdubey-star
Copy link
Member

/retest

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Cool! lets do it! 🎉

@ezr-ondrej ezr-ondrej merged commit cba059c into RHEnVision:main May 15, 2024
8 checks passed
@lzap lzap deleted the bump-go-1-21 branch May 15, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants