Skip to content

Conversation

@MrFreezeex
Copy link
Contributor

Mixins typically append .json or .yaml at the end of the name.
Remove those so that Grizzly can deploy them like the jsonnet code embed
into Grizzly does (grizzly.jsonnet).

Signed-off-by: Arthur Outhenin-Chalandre [email protected]

@malcolmholmes
Copy link
Contributor

Are you using fromMixin?

@MrFreezeex
Copy link
Contributor Author

Indirectly yes. I am using fromPrometheusKsonnet and I have all the mixins that I want in a mixins:: so this lib would eventually call fromMixin.

@MrFreezeex
Copy link
Contributor Author

MrFreezeex commented Feb 16, 2022

Hi @malcolmholmes, could we settle on something for this PR? It's literally a copy/paste of https://github.com/grafana/grizzly/blob/master/pkg/grizzly/grizzly.jsonnet#L8 and it would remove our internal hack to have grizzly working on mixins so I would really appreciate if this can be merged...

My other PR on grafana/grizzly (grafana/grizzly#199) is not a blocker for us so if you don't have time/you don't want to have it that's fine (see the two last comments in the PR for the latest context).

@malcolmholmes
Copy link
Contributor

Apologies for my delay.

This fix should not be made in the makeResource function, as this is not specific to the prometheus-ksonnet usecase.

The right place should be here:

fromMap(dashboards, folder):: {
[k]: util.makeResource('Dashboard', k, dashboards[k], { folder: folder })
for k in std.objectFields(dashboards)
},

e.g.:

fromMap(dashboards, folder):: {
    [k]: util.makeResource(
      'Dashboard',
      std.strReplace(std.strReplace(std.strReplace(k, '.json', ''), '.yaml', ''), '.yml', ''),
      dashboards[k],
      { folder: folder }
    )
    for k in std.objectFields(dashboards)
  }

If that works, I'll happily merge it!

@MrFreezeex MrFreezeex force-pushed the grizzly-name branch 2 times, most recently from 71424d0 to f746ec6 Compare February 28, 2022 08:29
@MrFreezeex
Copy link
Contributor Author

MrFreezeex commented Feb 28, 2022

Apologies for my delay.

No worries :).

This fix should not be made in the makeResource function, as this is not specific to the prometheus-ksonnet usecase.

The right place should be here:

fromMap(dashboards, folder):: {
[k]: util.makeResource('Dashboard', k, dashboards[k], { folder: folder })
for k in std.objectFields(dashboards)
},

Ah yes you are right thanks! I did it in the makeResource function definition instead because there are several calls to this function that should be fixed (i.e.: in fromMap and fromMixins).

@malcolmholmes
Copy link
Contributor

Ah yes you are right thanks! I did it in the makeResource function definition instead because there are several calls to this function that should be fixed (i.e.: in fromMap and fromMixins).

Yeah, but the makeResource is intended as a "new generation" thing, whereas the fromMap and fromMixins are the "migration from legacy". So the fix you're talking about needs to be in the latter so as not to effect the former.

Mixins typically append .json or .yaml at the end of the name.
Remove those so that Grizzly can deploy them like the jsonnet code embed
into Grizzly does (grizzly.jsonnet).

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@MrFreezeex
Copy link
Contributor Author

MrFreezeex commented Mar 2, 2022

Yeah, but the makeResource is intended as a "new generation" thing, whereas the fromMap and fromMixins are the "migration from legacy". So the fix you're talking about needs to be in the latter so as not to effect the former.

Ah ok I see. I added that in every fromMap/fromMixins call then!

@malcolmholmes
Copy link
Contributor

thanks for persisting!

@malcolmholmes malcolmholmes merged commit 70b2a1c into grafana:master Mar 7, 2022
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.

2 participants