Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Don't use globals for platforms and components registration #1147

Merged
merged 19 commits into from
Dec 3, 2020
Merged

Don't use globals for platforms and components registration #1147

merged 19 commits into from
Dec 3, 2020

Conversation

invidian
Copy link
Member

@invidian invidian commented Oct 29, 2020 •
edited
Loading

This pull request includes commits from #1098, please do not review commits from #1098 in this pull request

Closes #992
Closes #597

See commit messages for details.

@invidian invidian changed the title Don't use globals for platforms registration Don't use globals for platforms and components registration Oct 29, 2020
@invidian invidian marked this pull request as draft October 29, 2020 16:51
@invidian invidian force-pushed the invidian/globals branch 3 times, most recently from 63cc1eb to 1876094 Compare November 2, 2020 20:28
@invidian invidian marked this pull request as ready for review November 2, 2020 21:27
@invidian invidian requested review from ipochi and surajssd November 2, 2020 21:27
knrt10
knrt10 previously requested changes Nov 3, 2020
View reviewed changes
Copy link
Member

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Awesome work 🥳 . Some suggestions.

pkg/platform/aws/aws.go Outdated Show resolved Hide resolved
pkg/platform/baremetal/baremetal.go Outdated Show resolved Hide resolved
pkg/platform/baremetal/baremetal.go Outdated Show resolved Hide resolved
pkg/platform/packet/packet.go Outdated Show resolved Hide resolved
pkg/components/cluster-autoscaler/component.go Show resolved Hide resolved
pkg/components/external-dns/component.go Show resolved Hide resolved
pkg/components/linkerd/component.go Outdated Show resolved Hide resolved
@invidian invidian requested a review from knrt10 November 3, 2020 07:40
This was referenced Nov 11, 2020
Closed
Closed
Closed
@invidian invidian added the priority/P3 Low priority label Nov 17, 2020
@invidian invidian requested a review from iaguis November 27, 2020 09:23
surajssd
surajssd previously requested changes Nov 30, 2020
View reviewed changes
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

We also need to parallelize the tests.

pkg/platform/aws/aws.go
@@ -88,9 +88,14 @@ type config struct {
KubeAPIServerExtraFlags []string
}

const (
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason behind keeping the similar change in AKS module in a different commit? Can't this commit f725518 and the previous one 3e7898c coalesced into one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Commit as doing different things. One is exposing things, other is adding new exported const. Both commit still do one thing each, just in multiple contexts. I think I prefer to keep them separate.

cli/cmd/cluster/cluster.go Show resolved Hide resolved
test/components/install_test.go Outdated Show resolved Hide resolved
pkg/components/dex/component_test.go Show resolved Hide resolved
@invidian
Copy link
Member Author

We also need to parallelize the tests.

Which ones? I'm almost sure this is out of scope of this (already large) PR.

If you talk about the unit tests using configuration, I think we should fix them either on the way OR in a separate PR as part of other issue.

@surajssd addressed remaining review comments, please have a look again.

@invidian invidian requested a review from surajssd November 30, 2020 09:12
@surajssd
Copy link
Member

surajssd commented Dec 1, 2020

If you talk about the unit tests using configuration, I think we should fix them either on the way OR in a separate PR as part of other issue.

The promise of this change is that tests could be parallelised which weren't because we were extracting the same object from global store. So how do we confirm that promise is fullfilled with this change?

@invidian
Copy link
Member Author

invidian commented Dec 1, 2020

The promise of this change is that tests could be parallelised which weren't because we were extracting the same object from global store. So how do we confirm that promise is fullfilled with this change?

There is no global registry anymore and it's up to the package itself to ensure immutability, as NewConfig() function returns a pointer to configuration. Each package must ensure this pointer is unique, not taken from global, which is the case. It will be fine :)

@invidian
Copy link
Member Author

invidian commented Dec 1, 2020

I need to rebase.

@invidian
pkg/platform/aks: add NewConfig function to align with other platforms
ea64f95 
As a preparation for solving #992.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/platform/aks: export Name const
2bdf16b 
As a preparation for #992.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/platform/{aws,baremetal,packet,tinkerbell}: add Name const
79ad7c7 
As a preparation to #992, so platform name from the configuration don't
need to be duplicated.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
Don't use globals for registering platforms configuration structs
392d1e9 
This commit moves platforms registration logic from using globals to
single function in cli/cmd/cluster package, as this is the only user of
old GetPlatforms() function.

For platform unit tests, NewConfig() function exported by each platform
package should be used instead.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd/cluster: remove unused anonymous imports
6fd7981 
We no longer rely on the init function call registration method for all
platform packages, so anonymous imports are no longer needed.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian added 14 commits December 3, 2020 10:58
@invidian
pkg/components: export component names using Name constant
633ea1a 
As a preparation for #992.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components: rename newComponent() functions to NewConfig()
0988eb5 
To make it consistent with pkg/platform/* packages and to make it
available outside of package, so we can avoid using globals and init()
functions for collecting available components default configurations.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components/flatcar-linux-update-operator: add NewConfig function
3aaba38 
To be consistent with other components.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
test/components: use NewConfig() instead of components.Get()
26d5121 
So no globals or HCL parsing is involved in testing. Also
components.Get() is now deprecated, as we want to avoid relying on
globals and init() functions.

Part of #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components: use NewConfig() when getting default config for tests
1015951 
So we don't rely on other packages while running unit tests.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd/cluster: drop dependency on pkg/components.ListNames()
0d0db10 
Instead, add few helper functions, which will also replace
components.Get().

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd/cluster: use local componentConfig() instead of components.Get()
ac5dd1c 
As a next step of not using globals and init() functions across our
code.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components: remove Register(), Get() and other related functions
f3403b2 
They are not used anymore, so can be removed.

Refs #992.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/backend: rename functions returning new config to NewConfig()
b5e3db0 
To be consistent with platform and component packages.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/backend: export Name const for consistency
7c2c0dc 
To be consistent with platform and component packages.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd/cluster: don't use backend.GetBackend for getting backend config
1a039f6 
Import all available backends locally instead, to avoid using globals.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd/cluster: move Backend interface definition to only consumer
c435f74 
No other packages really need backend.Backend interface, so we can move
it to cli/cmd/cluster package where it is actually consumed, which is an
recommendation in Go. This way we can further reduce exported API of the
codebase.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
Remove pkg/backend package
b9793df 
We no longer use registration pattern for backends, as it is using
globals and init() which we want to avoid using.

We also moved the interface to cli/cmd/cluster package, as it was
the only consumer of it, so actually the entire package can be now
removed, as it no longer has any use.

Refs #992

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd/cluster/cluster.go: remove unused anonymous import
f728c60 
We no longer rely on the init function call registration method for all
backend packages, so anonymous imports are no longer needed.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
Copy link
Member Author

invidian commented Dec 3, 2020

Rebased to also adapt Tinkerbell platform for introduced changes.

iaguis
iaguis approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for getting rid of all those terrible init() functions!

cli/cmd/cluster/component.go Show resolved Hide resolved
@invidian invidian dismissed stale reviews from surajssd and knrt10 December 3, 2020 11:33

Addressed.

@invidian invidian merged commit c575337 into master Dec 3, 2020
@invidian invidian deleted the invidian/globals branch December 3, 2020 16:40
@iaguis iaguis mentioned this pull request Mar 5, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iaguis iaguis iaguis approved these changes

@ipochi ipochi Awaiting requested review from ipochi

@knrt10 knrt10 Awaiting requested review from knrt10

@surajssd surajssd Awaiting requested review from surajssd

Assignees
No one assigned
Labels
priority/P3 Low priority
Projects
None yet
Milestone
No milestone
4 participants
@invidian @surajssd @iaguis @knrt10