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

Fix jsonnet unhide for string values #88

Closed
wants to merge 1 commit into from

Conversation

thlacroix
Copy link

In the previous version, the jsonnet script to extract private elements didn't work for keys that have string values, like grafanaDashboardFolder.

The script did something like:

{
  grafanaDashboardFolder:: 'test',
} + {
  grafanaDashboardFolder+::: {}
}

and in this case, jsonnet only appends { } as a string to the grafanaDashboardFolder value.
Also, all jsonpath from handlers were forced to appear, even if they are not in the source script.

The new script uses a function with the source as argument to check if the field already exists, and force displays it in this case only. The other benefit is that no other handlers keys will be added to the output.

@malcolmholmes
Copy link
Collaborator

Thanks for this!

The whole idea of extracting hidden elements from Jsonnet is broken - you're just showing one issue amongst many! The right approach is to expose the hidden elements as actual elements from within Jsonnet itself. I've made a library to do this here. And I have a PR that implements this here: #72 . That PR did too many things, and won't be merged. I'm gonna hopefully make another start on it tonight.

In the meantime, it is important we support the old hiddens way, and I'll take a look at this before starting. Thanks again!

Bu

@malcolmholmes
Copy link
Collaborator

Are you sure this doesn't work? When I run Grizzly with the test data, the dashboard ends up in the sample folder. In what way are you seeing this not work?

@thlacroix
Copy link
Author

Thanks for having a look :)

Before this change, https://github.com/grafana/grizzly/pull/88/files#diff-83b82171360f543df501efc9342cc3475ef71208603a96dba97d096a06598d3dL90 was added to remove the { } suffix, which explains why the folder name appears correct down the line.

However this still mutates the grafanaDashboardFolder, so using $.grafanaDashboardFolder in dashboards.json would give the result with the wrong suffix.

Also then the grafanaDashboardFolder is discarded for dashboard creation, but I've made a separate fix for that #85

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@theSuess theSuess changed the base branch from master-legacy to main December 29, 2023 12:58
@theSuess theSuess requested a review from a team as a code owner December 29, 2023 12:58
@julienduchesne julienduchesne removed the request for review from a team January 11, 2024 01:47
@malcolmholmes
Copy link
Collaborator

Grizzly has changed a lot since this PR. Closing, on the assumption this is fixed. Please feel free to re-open.

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