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

Adds rule to check if each panel has valid units assigned #75

Merged
merged 11 commits into from
Jun 27, 2022

Conversation

mshahzeb
Copy link
Contributor

Reference issue:
#51

This rule checks that each panel must have a valid unit assigned.
The unit is extracted from dashboard json in the following manner:

  • Parse overrides to check if any override unit is present
  • If no overrride unit - then extract standard option unit
  • Check if unit is defined and is valid

Valid units are extracted from here:
https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/valueFormats/categories.ts

Considerations:

  • For a panel with multiple fields and panels - the last override unit will be picked

Sample output:
image

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2022

CLA assistant check
All committers have signed the CLA.

@mshahzeb
Copy link
Contributor Author

Since the override value is of dynamic type - It can be an int or a string.

I am getting an error in case when a dashboard has mixed int and string values in overrides.
I am working on a fix for that too.

// Enumerated from: https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/valueFormats/categories.ts

// Misc
"none", "string",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we leave none as an allowed unit to be set if no other unit makes sense?

Or remove this and let the user add a lint exclusion in that case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't sure the behavior for a default dashboard/panel, so I just tested it.

When a new panel is created, with no config, no unit is set. Literally the unit property is absent in fieldConfig.

In the dropdown/picker for units, none does not appear to be an option. If you type it in manually, you can set it as a "custom" unit.

For this reason, I would say this shouldn't be an allowed valid unit, and if users choose to explicitly specify "none" they should also create a lint exclusion rule.

Copy link
Collaborator

@rgeyer rgeyer left a comment

Choose a reason for hiding this comment

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

LGTM, with the one bit of feedback on the "none" unit type.

Great work. Thank you!

// Enumerated from: https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/valueFormats/categories.ts

// Misc
"none", "string",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't sure the behavior for a default dashboard/panel, so I just tested it.

When a new panel is created, with no config, no unit is set. Literally the unit property is absent in fieldConfig.

In the dropdown/picker for units, none does not appear to be an option. If you type it in manually, you can set it as a "custom" unit.

For this reason, I would say this shouldn't be an allowed valid unit, and if users choose to explicitly specify "none" they should also create a lint exclusion rule.

@mshahzeb mshahzeb merged commit 157c65f into main Jun 27, 2022
@mshahzeb mshahzeb deleted the shahzeb/add-panel-units-rule branch June 27, 2022 19:31
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.

None yet

3 participants