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

Add support for Contexts #271

Merged
merged 17 commits into from
Nov 30, 2023
Merged

Add support for Contexts #271

merged 17 commits into from
Nov 30, 2023

Conversation

malcolmholmes
Copy link
Contributor

At present, Grizzly is configured through envvars. This is great for CI setups (e.g 12 factor apps), and perhaps if you only interact with a single backend. However, as soon as you have more than one backend, it becomes painful.

This PR adds support for "contexts", modelled after kubectl. Each context contains a full set of settings/credentials. Switching between them becomes trivial.

@malcolmholmes malcolmholmes requested a review from a team November 27, 2023 10:26
@malcolmholmes malcolmholmes changed the title Malcolmholmes/add contexts Add support for Contexts Nov 27, 2023
Copy link
Member

@Duologic Duologic left a comment

Choose a reason for hiding this comment

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

Great to see these different auth settings part of the same context, it provides a sense of order. The commands to configure a context is something that could be a page on Grafana Cloud, just copy paste and ready to go!

Perhaps also worth bringing this up with the auth team, this might be a feature they could own eventually.

docs/content/authentication.md Outdated Show resolved Hide resolved
docs/content/authentication.md Outdated Show resolved Hide resolved
docs/content/authentication.md Outdated Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
pkg/grafana/provider.go Outdated Show resolved Hide resolved
pkg/grafana/provider.go Outdated Show resolved Hide resolved
@malcolmholmes
Copy link
Contributor Author

Great to see these different auth settings part of the same context, it provides a sense of order. The commands to configure a context is something that could be a page on Grafana Cloud, just copy paste and ready to go!

Perhaps also worth bringing this up with the auth team, this might be a feature they could own eventually.

All this does is brings together the way it is done now. There is a conversation to be had with them about whether we can reduce the number of auth settings at all.

@malcolmholmes malcolmholmes requested review from a team and Duologic November 29, 2023 19:52
docs/content/authentication.md Outdated Show resolved Hide resolved
Comment on lines +22 to +23
> **NOTE**: If you have used Grizzly previously with environment variables, you can
initialise the `default` context from your environment simply with `grr config import`.
Copy link
Member

Choose a reason for hiding this comment

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

So, Grizzly does not work without using that command first? Isn't that a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, envvars override contexts. So this is a way to transfer your envvars. What we need to document is that you then need to unset your envvars otherwise they will continue to override.

@@ -11,22 +11,12 @@ import (
)

func TestDashboard(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any tests that run the actual grr commands? I feel like these tests are slightly short of testing the whole thing and it'd actually be more useful (full integration test) and easier to call the grr commands to do the work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, would be great. We have a grafana instance started already. With the openapi client we can validate results. Perhaps this wouldn't be that hard to do actually.

Comment on lines +33 to +35
viper.BindEnv("overrides.mimir.address", "CORTEX_ADDRESS")
viper.BindEnv("overrides.mimir.tenant-id", "CORTEX_TENANT_ID")
viper.BindEnv("overrides.mimir.api-key", "CORTEX_API_KEY")
Copy link
Member

Choose a reason for hiding this comment

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

For a next PR, we could support MIMIR_ envvars and fallback to CORTEX_ ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, switch to calling out to mimirtool, which didn't exist when this integration was made. Then we could allow both CORTEX_ and MIMIR_ evvars, for back compat.

@malcolmholmes malcolmholmes merged commit da5092e into master Nov 30, 2023
3 checks passed
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