Skip to content
This repository was archived by the owner on Mar 6, 2025. It is now read-only.

Conversation

@nodehatch
Copy link
Contributor

Description

What does this PR do?

PR checklist for examples

Check these items before merging the PR.

  • Each example contains a README file.
  • README documents all steps and variables required to run the example. The simplest way to confirm this is to follow the instructions on a clean machine.
  • README file contains the links to the official documentation with relevant configuration and to the metrics published if that is available.
  • Used images of apps and collectors have to be pinned, "latest" should never be used.
  • Evidences with screenshots provided from the reviewer.
  • Includes a file called metrics.csv with metrics produced by the example.
    The file should have these 5 headings: Name, Description, Unit, DataType, Attributes.
    Description is not provided for all metrics, so it may be blank. Attributes are also not always provided. When there are multiple attributes on a metric reord they should be space separated.
  • Dashboard provided with example and put to the folder "dashboard" inside the example folder.
  • At least one screenshot of the proposed dashboard is included in this PR. If you're proposing substantive changes to queries or new queries then please ensure your screenshot shows relevant data.
  • All chart names in the dashboard are in Title case.
  • All query names in the dashboard are lower case letters, beginning the first query of each chart as "a" and proceeding alphabetically ... "b", "c", etcetera.

@nodehatch nodehatch self-assigned this Sep 14, 2023
Copy link
Contributor

@nslaughter nslaughter left a comment

Choose a reason for hiding this comment

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

Looks really good.

Only thing I would say here is instead of using the Makefile to use the opa image to run the build command. You could do this in an init container that the main opa depends on.

If this doesn't make sense then not a big deal and just let me know. I can write it out.

Co-authored-by: Nathan Slaughter <28688390+nslaughter@users.noreply.github.com>
@nodehatch
Copy link
Contributor Author

Looks really good.

Only thing I would say here is instead of using the Makefile to use the opa image to run the build command. You could do this in an init container that the main opa depends on.

If this doesn't make sense then not a big deal and just let me know. I can write it out.

I can apply this in the dashboard PR

@nslaughter
Copy link
Contributor

Looks really good.
Only thing I would say here is instead of using the Makefile to use the opa image to run the build command. You could do this in an init container that the main opa depends on.
If this doesn't make sense then not a big deal and just let me know. I can write it out.

I can apply this in the dashboard PR

Can't think of many reasons why we would apply some improvements to this code in the dashboard PR. So I'm going to guess that your motivation is to optimize throughput by accepting some imperfections that we can touchup without breaking momentum. If that's the goal then I'm happy to cosign for that! :shipit:

@nslaughter
Copy link
Contributor

When I revisited I realized a couple of things...

  1. You already ran the opa tool and committed built policy, so I removed that step in the Makefile as I think this makes the approach reasonably simpler - and that reduced the Makefile to one rule that just ran docker-compose.
  2. I tweaked the README to reflect that the policy was prebuilt for the demo.

So for this there's no reason to mess with the bundle builder container. Users interested in the example will understand the opa setup from scratch separately.

Thanks again. Really like how efficient we kept this one.

@nslaughter nslaughter enabled auto-merge (squash) September 15, 2023 20:20
@nslaughter nslaughter self-requested a review September 15, 2023 20:21
Copy link
Contributor

@nslaughter nslaughter left a comment

Choose a reason for hiding this comment

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

LGTM

@nslaughter nslaughter disabled auto-merge September 15, 2023 20:23
@nslaughter nslaughter merged commit 2f56caa into main Sep 15, 2023
@nslaughter nslaughter deleted the feat/collector/opa branch September 15, 2023 20:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants