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

Scripts: Add quickstart script for alert generator compliance test #5441

Closed
wants to merge 7 commits into from

Conversation

matej-g
Copy link
Collaborator

@matej-g matej-g commented Jun 24, 2022

Signed-off-by: Matej Gera matejgera@gmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

We have recently introduced the alert generator compliance test via E2E (see #5315). While this is useful for being able to run the test as part of our E2E suite, I want to propose also adding a separate script to spin up all components for the test run. Couple of reasons why:

  • We want to provide example configuration in the alert generator compliance repo to signal to users that Thanos is compliant. But because the E2E is running inside Docker, the configuration we have in the test is transient and cannot be easily made into a 'runnable' example config
  • Users do not need to look within our test files (and having to uncomment the skip call etc.), all is spun up with one script
  • Users do not even have to have Go installed (as opposed to E2E test), they can directly run this test with virtually no extra hurdles
  • Test is more transparent in a sense that users can observe the behavior (look at the web UIs) of the components, since these are running on the host machine (as opposed to containers without exposed ports).

Verification

Tested the alert generator compliance suite with the script on my local machine.

Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
saswatamcode
saswatamcode previously approved these changes Jun 28, 2022
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks! Awesome work! 🌟

Signed-off-by: Matej Gera <matejgera@gmail.com>
Copy link
Contributor

@jessicalins jessicalins left a comment

Choose a reason for hiding this comment

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

cool! ❇️
I tested the script locally and it seems to work fine :)
left a small suggestion

scripts/alert-compliance-quickstart.sh Outdated Show resolved Hide resolved
scripts/alert-compliance-quickstart.sh Outdated Show resolved Hide resolved
scripts/alert-compliance-quickstart.sh Show resolved Hide resolved
jessicalins
jessicalins previously approved these changes Jun 30, 2022
Copy link
Contributor

@jessicalins jessicalins left a comment

Choose a reason for hiding this comment

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

LGTM, awesome work 🎉
I think last thing would be to fix the commit sign off

matej-g and others added 2 commits June 30, 2022 11:53
Co-authored-by: Jéssica Lins  <jlins@redhat.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
@bwplotka
Copy link
Member

Hm, I would like to challenge this a little bit. I used the e2e on purpose, so it's actually more reliable for us to run, so maintain, and perhaps run nightly someday. Going through your arguments:

  • We want to provide example configuration in the alert generator compliance repo to signal to users that Thanos is compliant. But because the E2E is running inside Docker, the configuration we have in the test is transient and cannot be easily made into a 'runnable' example config

I think passing just the example config we autogenerate is good enough. Many of those solutions are managed services, so you can't easily provide scripts anyway. See:

  • Users do not need to look within our test files (and having to uncomment the skip call etc.), all is spun up with one script
  • Test is more transparent in a sense that users can observe the behavior (look at the web UIs) of the components, since these are running on the host machine (as opposed to containers without exposed ports).

Right, but who is a user. Is there a requirement for any user to run compliance on Thanos? The user to me is just Thanos maintainers responsible for it, so we can apply our processes. Plus you can experiment with e2e too - perhaps we need better documentation on how to do it (add e2einteractive.RunUntil... and check exposed ports in logs, or use container.Endpoint(."http"))

  • Users do not even have to have Go installed (as opposed to E2E test), they can directly run this test with virtually no extra hurdles

They still do - how else they build thanos binary, we use in this script?

To sum up

I don't strictly oppose here (:

Looks like team is happy here - but I just don't see why we would need to maintain the configuration of the extra script in different way we test other things.

Alternatives:

  1. Perhaps adding this as example script to https://github.com/prometheus/compliance/blob/main/alert_generator and our file as a comment makes sense?
  2. Adding steps or script that runs compliance go test. Remove Skip and put a special build tag that triggers the test?

@matej-g
Copy link
Collaborator Author

matej-g commented Jun 30, 2022

@bwplotka those are good counterarguments (I also do not want to push this change at all costs, as it is extra stuff to maintain, but still thought it would be a nice suggestion). To give more context how I was thinking about this:

  • For a developer, the E2E tests are useful: I make some changes and I want to make sure we are still compliant = great, I can run this E2E test
  • For any other user (e.g. anyone who just wants to run / play with Thanos) and they want to see for themselves that we are compliant (before they decide to use us vs. other solution, for example): They don't need to clone the repo (need just one script), they don't need Go or build Thanos (using latest release is fine), they should simply be able to run the script + alert compliance tester and be able to immediately and transparently see we are compliant.

I think I look at the compliance to be more for users to be able to transparently verify we comply with the official specification, more than for developers (but who also need it if they make relevant change). It's kind of a 'certification' to me and often it's good to provide an easy way to verify that the certification is valid for anyone who wants to challenge it.

Perhaps adding this as example script to https://github.com/prometheus/compliance/blob/main/alert_generator and our file as a comment makes sense?

I was thinking about that as well, and we could try if the compliance suite maintainers are fine with it, it's just quiet a few lines so it seemed liked it would be better if it's separate script. But we could try this alternative as well.

Adding steps or script that runs compliance go test. Remove Skip and put a special build tag that triggers the test?

That would be fine as well, although users would still need to clone the repo, have Go etc. (point I addressed above, but maybe it's not that strong of a hurdle).

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Don't want to block this, it's not a big harm in having the extra script, just it might get obsolete pretty soon.

@matej-g
Copy link
Collaborator Author

matej-g commented Jul 11, 2022

I don't think we have a strong enough agreement on this, I don't want to introduce something that might not be really needed / might become obsolete fast. I'd rather close this and if we decide to introduce the script, whether here or in the compliance repo, we can reuse this PR 👍.

@matej-g matej-g closed this Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants