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

updated sdic to not print anything and use the logging module, ran black #7

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

dimitrylukashov
Copy link

@dimitrylukashov dimitrylukashov requested a review from lra January 14, 2019 22:23
@dimitrylukashov
Copy link
Author

@lra how do we remove flake8 from here?

Copy link
Member

@lra lra left a comment

Choose a reason for hiding this comment

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

We should not switch from printing to logging. We should add an option on the commandline to switch and change the default, the default should always be interactive mode (launched by user) so that it's easy for users to try things out.

Also, I'm not sure syslog is the right recipient for it, as it's usually a multiline output. Why not letting the user what to do with the output? With stdout, he can run it, email the output, syslog it, ... We could just document sample use case if you think users don't know this stuff.

This would be better than forcing all use cases to use syslog.

docker:
- image: python:2.7
- image: 667005031541.dkr.ecr.us-west-1.amazonaws.com/black:18.9b0
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a public image? We don't want to use private stuff in our public repo to prevent leaking (anyone can fork, open a PR, and dump some env vars)

`[email protected]` is the email that will get the soft constraints broken every
day. Make sure your local MTA is well configured on your system. You can test
it by doing `date | mail -s test [email protected]`.
In Papertrail `sdic.enforce_fullname` will be treated as a
Copy link
Member

Choose a reason for hiding this comment

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

Papertrail is really specific to us, can you talk about syslog in general instead?

@lra
Copy link
Member

lra commented Jan 14, 2019

When the PR is ready to be merged, I can set github to replace the flake8 test by black.

@dimitrylukashov
Copy link
Author

@lra the primary use case of this tool is to check the integrity of bad data in a production database. Developers should not be manually using this tool at all, and that use case is not described in the README. The tool is also not usable for individual developer's use as you cannot isolate to a specific check, you have to run all the jobs per server at a time. It would be trivial, of course, to modify this, but developer's should not be encouraged to manually run scripts until we have kubernetes jobs or some other equivalent.

As is, sdic is neither usable or scalable. Emailing output from cron is also a bad pattern, insecure, and a ridiculous way of distributing results to individual teams. By setting up papertrail alerts each team can keep track of their own checkers.

I agree that multi-line output is a challenge but I would rather fitz with the format than deal with email. Feel free to propose any other alerting solution.

@lra
Copy link
Member

lra commented Jan 14, 2019

Developers should not be manually using this tool at all, and that use case is not described in the README. The tool is also not usable for individual developer's use as you cannot isolate to a specific check, you have to run all the jobs per server at a time. It would be trivial, of course, to modify this, but developer's should not be encouraged to manually run scripts until we have kubernetes jobs or some other equivalent.

This is not only about devs, this is about anyone using the tool. When you write tools, you should always focus on making it easy to use with sane defaults. By keeping sdic easy to run on the command line, you make it easy to run on every env out there (be it a cron, a container or a k8s job) and easy to test and to learn.

When a tool requires outside state (e.g. syslog output), you lose users down the line as the tool is asking users to change their environment to be able to be used. Think unix commands.

As is, sdic is neither usable or scalable. Emailing output from cron is also a bad pattern, insecure, and a ridiculous way of distributing results to individual teams. By setting up papertrail alerts each team can keep track of their own checkers.

sdic is using stdout, which is the standard in unix: it can be used as is on the command line, or piped to a cron, to an email, to syslog, or even to pagerduty. By hardcoding the output to syslog, you rule out all the other possibilities.

I agree that multi-line output is a challenge but I would rather fitz with the format than deal with email. Feel free to propose any other alerting solution.

Whatever solution fits you, you can do everything by piping an stdout output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants