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

Allow multiple data sources, of different types. Still enforce best-practice naming. #124

Merged
merged 4 commits into from
May 2, 2023

Conversation

rgeyer
Copy link
Collaborator

@rgeyer rgeyer commented Apr 26, 2023

This adds the flexibility in data source naming which is discussed in #14.

Specifically, if a dashboard has only one data source, it may have the name/uid datasource or <type>_datasource, and it may have the label "Data Source" or "<Title(type)> Data Source".

This also ensures that when more than one data source is present on the dashboard, the more specific name is no longer optional, but required.

Also, this tweaks the template-job-rule and template-instance-rule to allow for either form of the prometheus data source UID/name.

Finally, this tweaks the panel-datasource-rule to allow the use of any data source which is actually defined on the dashboard, rather than only checking for $datasource and ${datasource}.

Resolves #14 and supersedes #101 and #40

Copy link

@StefanKurek StefanKurek left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell. Didn't take the full time to read through the supporting code to fully understand the effects of the changes, so just commenting rather than approving 😄 . Will be nice to get rid of some of the linter exceptions for sure.

Copy link

@schmikei schmikei left a comment

Choose a reason for hiding this comment

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

This looks pretty good! I think that we'll run into quite a few misnames of Prometheus Data Source && Loki Data Source once this gets incorporated.

i.e. a lot of mixing and matching Loki Datasource && Data Source from a quick scan.

Was just curious is the Data Source supposed to be capitalized or is it supposed to be sentence case in the UI element if we are following this guideline?
https://grafana.com/docs/writers-toolkit/style-guide/ux-writing/#use-sentence-case-in-ui-elements

I'll defer to you but I was just curious for my own sake of knowledge :)

@rgeyer
Copy link
Collaborator Author

rgeyer commented Apr 27, 2023

Was just curious is the Data Source supposed to be capitalized or is it supposed to be sentence case in the UI element if we are following this guideline? https://grafana.com/docs/writers-toolkit/style-guide/ux-writing/#use-sentence-case-in-ui-elements

I'll defer to you but I was just curious for my own sake of knowledge :)

Ahh, good point, it may be properly Data source given that standard.

I'll check with our docs/standards team to be sure. Thanks for the heads-up, and for giving it a look!

@JonathanWamsley
Copy link

Screenshot 2023-04-27 at 3 36 25 PM
There also seems to be some linting inconsistencies with job wanting to be lowercased too while others seem to be fine.

@rgeyer
Copy link
Collaborator Author

rgeyer commented Apr 27, 2023

Screenshot 2023-04-27 at 3 36 25 PM There also seems to be some linting inconsistencies with job wanting to be lowercased too while others seem to be fine.

Yeah, job and instance have always had a rule which requires them to be lower case.

Which... Does seem a bit inconsistent. I'll have to think through this. I'd rather not change it outrightly, since that would cause sudden failures for anyone who's been following the rules as-is.

But I think you're right, in the spirit of sentence casing, those labels should probably be capitalized.

…ty. Update job and instance rules for sentence case.
@rgeyer
Copy link
Collaborator Author

rgeyer commented Apr 27, 2023

In the most recent commit, I actually make a couple of changes which should promote backward compatibility, and ease the move forward.

For one, since the formatting of the labels for data sources have changed, this is downgraded to a warning. In a future version (we don't have a canonical version for the linter yet, but maybe at 1.0?), we can change this back to an error.

Also, title casing for Job and Instance are actually correct on the label, so the rule enforces that now. However, for those who were using the dashboard-linter previously, and may have labeled all of their job and instance templates in lower case, the rule is now a Warning when it's violated, which should allow an easier transition.

This probably also warrants a changelog of sorts for the dashboard-linter, so that these sorts of changes/compromises can be documented.

Copy link

@v-zhuravlev v-zhuravlev left a comment

Choose a reason for hiding this comment

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

Well thought out! LGTM

@rgeyer rgeyer merged commit b1f5eb2 into main May 2, 2023
6 checks passed
@rgeyer rgeyer deleted the rgeyer/multiple-ds-types branch May 2, 2023 15:33
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.

Datasource selection priority
5 participants