This repository has been archived by the owner on Jun 29, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 49
Lokoctl uses global variable which gives wrong results if used via APIs #992
Labels
bug
Something isn't working
size/m
Issues which likely require up to a couple of work days
technical-debt
Technical debt-related issues
Milestone
Comments
invidian
added
proposed/next-sprint
Issues proposed for next sprint
technical-debt
Technical debt-related issues
labels
Sep 22, 2020
johananl
added
the
size/m
Issues which likely require up to a couple of work days
label
Sep 23, 2020
As this is already addressed PR of @invidian. Would you mind if this is assigned to you? |
invidian
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for solving #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Oct 29, 2020
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
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for solving #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Oct 29, 2020
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
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for solving #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Oct 29, 2020
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
added a commit
that referenced
this issue
Oct 29, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
Okay, this again turns out to be never-ending story to do right:
Edit:
Platforms already have However, the problem still applies for components. I guess they could follow the same convention as platforms for now. |
invidian
added a commit
that referenced
this issue
Nov 2, 2020
As a preparation for solving #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Nov 2, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Nov 2, 2020
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
added a commit
that referenced
this issue
Nov 2, 2020
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
added a commit
that referenced
this issue
Nov 2, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
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
added a commit
that referenced
this issue
Dec 2, 2020
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
added a commit
that referenced
this issue
Dec 2, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
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
added a commit
that referenced
this issue
Dec 2, 2020
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
added a commit
that referenced
this issue
Dec 2, 2020
So we don't rely on other packages while running unit tests. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
Instead, add few helper functions, which will also replace components.Get(). Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
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
added a commit
that referenced
this issue
Dec 2, 2020
They are not used anymore, so can be removed. Refs #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
To be consistent with platform and component packages. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
To be consistent with platform and component packages. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
Import all available backends locally instead, to avoid using globals. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 2, 2020
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
added a commit
that referenced
this issue
Dec 3, 2020
As a preparation for solving #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
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
added a commit
that referenced
this issue
Dec 3, 2020
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
added a commit
that referenced
this issue
Dec 3, 2020
As a preparation for #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
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
added a commit
that referenced
this issue
Dec 3, 2020
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
added a commit
that referenced
this issue
Dec 3, 2020
So we don't rely on other packages while running unit tests. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
Instead, add few helper functions, which will also replace components.Get(). Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
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
added a commit
that referenced
this issue
Dec 3, 2020
They are not used anymore, so can be removed. Refs #992. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
To be consistent with platform and component packages. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
To be consistent with platform and component packages. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
Import all available backends locally instead, to avoid using globals. Refs #992 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
invidian
added a commit
that referenced
this issue
Dec 3, 2020
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>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
bug
Something isn't working
size/m
Issues which likely require up to a couple of work days
technical-debt
Technical debt-related issues
Consider following test code:
Whose output looks like this:
Notice that in second test case we should see
https://prometheus.mydomain.net
but the old result persists for some reason. My first guess is that this is due to use of globals.I think
component.LoadConfig
andcomponent.RenderManifests
don't do the right thing. Or we should flush out old values whencomponents.Get
is called.The text was updated successfully, but these errors were encountered: