Fixes before 0.2.0 release #150
Conversation
| @@ -96,6 +96,9 @@ func postDashboard(resource grizzly.Resource) error { | |||
| return err | |||
| } | |||
|
|
|||
| if !resource.HasMetadata("folder") { | |||
| return fmt.Errorf("Resource %s requires a folder within its metadata", resource.GetMetadata("name")) | |||
| } | |||
There was a problem hiding this comment.
complain if there is no folder metadata within a dashboard.
| @@ -52,7 +52,7 @@ func getRemoteRuleGroup(uid string) (*grizzly.Resource, error) { | |||
| "rules": group.Rules, | |||
| } | |||
| handler := RuleHandler{} | |||
| resource := grizzly.NewResource(handler.APIVersion(), handler.Kind(), uid, spec) | |||
| resource := grizzly.NewResource(handler.APIVersion(), handler.Kind(), group.Name, spec) | |||
There was a problem hiding this comment.
uid is $namespace.$name, so this misreports the name portion.
| if parsedResource.MatchesTarget(opts.Targets) { | ||
| resources = append(resources, parsedResource) | ||
| } | ||
| } |
There was a problem hiding this comment.
somehow we lost our -t target handling functionality when parsing with Jsonnet.
| @@ -9,6 +11,9 @@ const generalFolderId = 0 | |||
| func extractFolderUID(d DashboardWrapper) string { | |||
| folderUid := d.Meta.FolderUID | |||
| if folderUid == "" { | |||
| if d.Meta.FolderURL == "" { | |||
| return "general" | |||
| } | |||
There was a problem hiding this comment.
Grafana can come back with a blank folder URL for the general folder.
| @@ -27,10 +28,12 @@ func TestExtractFolderUID(t *testing.T) { | |||
| } | |||
| getFolderById = func(folderId int64) (Folder, error) { | |||
| return Folder{ | |||
| "uid": "12345", | |||
| "uid": "12345", | |||
| "folderURL": "http://localhost:3000/blah", | |||
| }, nil | |||
| } | |||
| uid := extractFolderUID(dashboardWrapper) | |||
| require.Equal(t, "12345", uid) | |||
There was a problem hiding this comment.
Need to also check for folderUrl here?
| @@ -21,7 +21,7 @@ func TestRules(t *testing.T) { | |||
| } | |||
| res, err := getRemoteRuleGroup("first_rules.grizzly_alerts") | |||
| require.NoError(t, err) | |||
| require.Equal(t, "first_rules.grizzly_alerts", res.Name()) | |||
| require.Equal(t, "grizzly_alerts", res.Name()) | |||
There was a problem hiding this comment.
It's not jumping out to me in the code what's changed that makes this test pass on both sides of the overall changeset, could you explain?
There was a problem hiding this comment.
Closing this PR (work done elsewhere), but to answer your question:
A resource is uniquely identified by its Kind and UID. Resources also have a name, but that is not going to be consistent across all resource types, 'cos for example a rule group can appear in multiple namespaces.
Therefore, I've been constructing a UID from various bits of metadata, e.g. $NAMESPACE.$NAME.
What does occur to me though is that people will expect metadata.name to be unique, so constructing a UID in the background really doesn't help here.
What would be much better would be to require the name to be constructed correctly, and validate it is correct, erroring out with a nice message if it isn't. So:
apiVersion: grizzly.grafana.com/v1alpha1
kind: PrometheusRuleGroup
metadata:
name: some_namespace.some_rules
spec:
name: some_rules
namespace: some_namespace
rules:
....all the rules...Here, metadata.name becomes a unique identifier, and validation in the handler confirms that it it matches ${spec.namespace}.${spec.name}.
|
This work has all been done in other PRs. |
see comments inline to explain these changes.