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

[WIP] PoC for managing Grafana App Platform resources #2006

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

radiohead
Copy link

What

This PR is a PoC for managing Grafana App Platform resources via Terraform. It implements a generic GAP resource, generic client and implements two new resources – dashboards and playlists.

The dashboard resource also supports validation (using grafana/foundation-sdk and linting using grafana/dashboard-linter.

This is still WIP and parts of this code will be move to other repositories (e.g. generated dashboards code will be moved to grafana/grafana, resource clients to grafana/grafana-app-sdk and so on).

Why

To add support for GAP resources and improve the existing Grafana dashboards resource specifically.

The code for specific resources should ideally be auto-generated by something (likely something similar or same as in https://github.com/grafana/terraform-provider-schemas/tree/main/gen).

Copy link

In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically.
To do so, a Grafana Labs employee must trigger the cloud acceptance tests workflow manually.

@radiohead radiohead force-pushed the feat/app-platform-poc branch from b6fef2c to 1fc73cd Compare January 28, 2025 14:53
Copy link
Author

Choose a reason for hiding this comment

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

I've added this as a temporary solution, to isolate all required changes in one place. However, this ended up requiring moving the common.Client from internal to pkg, because it wasn't otherwise possible to re-use it in this submodule.

If we are going to stick to the submodule approach, we'll need to think about a cleaner way to address the changes to the common.Client.

Copy link
Author

Choose a reason for hiding this comment

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

This is 90% there, just needs validation for item types (since those are an enum).

Copy link
Author

Choose a reason for hiding this comment

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

This is the main piece of work here, would be nice to get feedback on it.

Copy link
Author

Choose a reason for hiding this comment

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

These will be moved and imported from grafana/grafana (same way as playlists are imported right now).

Copy link
Author

Choose a reason for hiding this comment

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

@IfSentient I'm thinking about adding these 2 clients (resource & namespaced) to app SDK, thoughts?

Choose a reason for hiding this comment

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

I think that makes sense, I think we might want to rename the NamespacedClient, as it's actually a Namespaced ResourceClient, and grafana/grafana-app-sdk#194 will likely need to introduce a namespaced general client/registry.

Copy link
Author

Choose a reason for hiding this comment

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

Great! I'm happy to rename / change things slightly – what's here is definitely not final.

@radiohead
Copy link
Author

@Duologic @K-Phoen @IfSentient this is still WIP but if you have some time I'd appreciate general feedback for this!

Especially about the submodule approach – I'd like to know if we should stick with it or not.

@radiohead radiohead self-assigned this Jan 28, 2025
@Duologic
Copy link
Member

On first scan:

  • It looks like the refactor for the client could fit into a separate PR as it doesn't seem to change how it works.
  • I don't have experience with the submodule approach, not sure what the trade-offs are.
  • Can the dashboard schema be pulled in as a dependency from grafana/grafana instead of vendoring? From reading the description, I'd expect the SDK and linter need to know about the schema, not necessarily the provider directly, am I misunderstanding something?

@radiohead
Copy link
Author

Thank you for taking a look!

On first scan:

  • It looks like the refactor for the client could fit into a separate PR as it doesn't seem to change how it works.
  • I don't have experience with the submodule approach, not sure what the trade-offs are.

If you are OK with not using a submodule it will make client refactoring unnecessary – the only reason I've done it was to make it importable from a submodule.

Let me know if dropping the submodule is fine and I'll update accordingly.

  • Can the dashboard schema be pulled in as a dependency from grafana/grafana instead of vendoring? From reading the description, I'd expect the SDK and linter need to know about the schema, not necessarily the provider directly, am I misunderstanding something?

Yes, I'm working on moving the dashboard schemas to grafana/grafana and importing them the same way playlist schemas are currently imported.

@Duologic
Copy link
Member

Duologic commented Jan 31, 2025

I don't really understand which problem the submodule is solving tbh. What are the trade offs of dropping the submodule?

@radiohead
Copy link
Author

I don't really understand which problem the submodule is solving tbh. What are the trade offs of dropping the submodule?

The main reason I've introduced one was to isolate the changes and the dependencies. I am 100% happy to drop it.

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