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

cert-manager example (community hack session) #21

Merged
merged 6 commits into from
Jul 13, 2021
Merged

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Jul 6, 2021

Started working on converting this cert-manager mixin in a community hack session. Thanks to @rgeyer for his great questions!

Only got to one alert and a bunch of discussion around it, but that's OK!

Interesting possible change in here, though - this changes the structure of PollyPackage to make rules and alerts more addressable from the outside. That is, instead of having a list, they're effectively all in a string-keyed map.

This is an approach to the grouping problem i described to in #16. It does mean consuming tools will need to transform the alerts and rules in order to get them in the list form that Prom expects to see them - but i think that's acceptable. Thoughts, @metalmatze?

/cc @malcolmholmes - i know you wanted to map-ify these lists long, long ago

@sdboyer
Copy link
Member Author

sdboyer commented Jul 6, 2021

Will continue work on this on Friday

}

prometheusAlerts: v0: {
CertManagerCertExpirySoon: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maps! YISSSS! 🎉 🎉

Finally I will be able to stop iterating over the whole thing just to select one alert ^^

@paulfantom
Copy link
Contributor

It does mean consuming tools will need to transform the alerts and rules in order to get them in the list form that Prom expects to see them - but i think that's acceptable.

We'll need that tool (or good, documented examples) as part of polly org if we want to have widespread adoption.

//
// That's the ideal, anyway - we'll have to see what we can actually
// accomplish :)
dashboard_url: header.params.grafanaExternalUrl + "/d/TvuRo2iMk/cert-manager",
Copy link
Contributor

Choose a reason for hiding this comment

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

set a UID on your dashboard and you can get rid of that randomised string: TvuRo2iMk

schema/pollypkg.cue Outdated Show resolved Hide resolved
@sdboyer
Copy link
Member Author

sdboyer commented Jul 7, 2021

We'll need that tool (or good, documented examples) as part of polly org if we want to have widespread adoption.

Absolutely. I think the target here is probably docs + CUE helpers (i'm imagining more things in the "util" vein described over here) + impl in some tools.

I think the helpers will probably be most useful. Any translation layer that exists must necessarily start by consuming a pop, which likely means starting from the [Go] CUE SDK, where it will be possible to rely on these helpers directly. That means the option will always be there to have the alerts/rules arrive in the userspace for that tool (e.g. HCL for terraform, jsonnet files for jsonnet) in the canonical Prometheus form of a list. It may not be the best option for users, but having the CUE util bit should make it available, in a standard way, for any consuming toolchain, and the author of that consumer gets to make their own UX judgment.

Co-authored-by: malcolmholmes <[email protected]>
@sdboyer sdboyer force-pushed the hacksession-certmanager branch 2 times, most recently from a6bdcaf to 9b0a42f Compare July 9, 2021 17:54
@sdboyer sdboyer marked this pull request as ready for review July 9, 2021 18:35
@sdboyer
Copy link
Member Author

sdboyer commented Jul 9, 2021

OK, this is ready to go. All the alerts and the grafana dashboard are there. Some notes:

  • A little README is in there, a halfway stab at some context re: Create guidelines for helper docs on examples #24
  • I added the #FlattenAlerts helper function-struct, which should address @paulfantom's concern
  • The Grafana dashboard is really not representative of how things will eventually be, primarily because the Grafana dashboard schema just isn't mature yet. (We're working on that!). I go into more detail in the community hack time recording, starting around 22:00.

This return naming pattern complies with the standard CUE approach to
creating "functions."
@sdboyer sdboyer merged commit 9c60987 into main Jul 13, 2021
}
}

grafanaDashboards: v0: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor issue - would it make sense to structure code in a similar way we did it in mixins?

put dashboards in separate files? With a pretty small pollypkg containing just 1 dashboard we already have a 1k+ LOC and it might get too long to be readable in case of larger pollypkgs/mixins.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would, yes.

we discussed this a bit on the last hack session - file layout relates heavily to how we namespace (URI) and generate code from a UI. The rules here can be simple, I think - handwaving, but basically:

  • All pop files must be in the same directory
  • All files must be in the same CUE package (unclear if we specify what that package must be)
  • The emit value of the package must be an instance of PollyPackage
  • Files created by UI-driven tools should follow some suffix or prefix pattern
  • Files containing only some particular object type should follow some prefix or suffix pattern (we can lint this! 🎉)

Copy link
Member Author

Choose a reason for hiding this comment

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

These rules have the effect of making package generation and linting pretty straightforward (standard CUE rules for loading a whole dir's package), removing any (I think?) unnecessary choice, render any within-the-pop importing unnecessary (unlike today's mixins). It should all be smooth and clear.

If you could post an issue about this, I'll use the next example/hack session as an opportunity to either do a new pop that conforms to these patterns, or make a new one 😄

@paulfantom paulfantom deleted the hacksession-certmanager branch July 23, 2021 06:26
@paulfantom paulfantom restored the hacksession-certmanager branch July 23, 2021 06:26
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.

3 participants