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

Adding sqs_queue_status script to cron #731

Merged
merged 6 commits into from Aug 28, 2018

Conversation

Projects
None yet
3 participants
@Phrozyn
Contributor

Phrozyn commented Aug 6, 2018

I didn't do a json config for all accounts (we only have 2)
In the interest of time and needing to get to goal items, doing so would require more time and refactoring as it's something I am unfamiliar with.

@Phrozyn Phrozyn requested a review from pwnbus Aug 6, 2018

@zsck

zsck approved these changes Aug 13, 2018

for host in data:
for key in host['_source']['details'].keys():
# remove unwanted data
if '.' in key:

This comment has been minimized.

@zsck

zsck Aug 13, 2018

Just to make this easier to grok at a glance, I suggest we replace this "magic string" with a useful variable name.

details_delimiter = '.'
if details_delimiter in key:
@zsck

zsck Aug 13, 2018

Just to make this easier to grok at a glance, I suggest we replace this "magic string" with a useful variable name.

details_delimiter = '.'
if details_delimiter in key:

This comment has been minimized.

@Phrozyn

Phrozyn Aug 13, 2018

Contributor

Same as with the other comment goes for this one:
This is what is in place in the other scripts, I didn't deviate from what we have standardized on.
I would think if we want to change it, we should change it across all scripts in a separate PR.

@Phrozyn

Phrozyn Aug 13, 2018

Contributor

Same as with the other comment goes for this one:
This is what is in place in the other scripts, I didn't deviate from what we have standardized on.
I would think if we want to change it, we should change it across all scripts in a separate PR.

return hash.hexdigest()
def getQueueSizes():
logger.debug('starting')

This comment has been minimized.

@zsck

zsck Aug 13, 2018

Would it be useful for this to log something like "starting sqs_queue_status getQueueSizes"?

@zsck

zsck Aug 13, 2018

Would it be useful for this to log something like "starting sqs_queue_status getQueueSizes"?

This comment has been minimized.

@Phrozyn

Phrozyn Aug 13, 2018

Contributor

This is what is in place in the other scripts, I didn't deviate from what we have standardized on.
I would think if we want to change it, we should change it across all scripts in a separate PR.

@Phrozyn

Phrozyn Aug 13, 2018

Contributor

This is what is in place in the other scripts, I didn't deviate from what we have standardized on.
I would think if we want to change it, we should change it across all scripts in a separate PR.

This comment has been minimized.

@zsck

zsck Aug 21, 2018

I don't think we should continue using an arguably suboptimal style just because it's standardized. Adding more information here will only serve as an improvement and a reminder that we should make similar changes elsewhere. My experience, and it seems our team's experience, with delaying such changes for a future concerted effort is that this future effort is never made.

@zsck

zsck Aug 21, 2018

I don't think we should continue using an arguably suboptimal style just because it's standardized. Adding more information here will only serve as an improvement and a reminder that we should make similar changes elsewhere. My experience, and it seems our team's experience, with delaying such changes for a future concerted effort is that this future effort is never made.

@Phrozyn Phrozyn merged commit 44c38e2 into mozilla:master Aug 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Phrozyn Phrozyn deleted the Phrozyn:sqs_queue_status branch Aug 28, 2018

@pwnbus pwnbus added this to the Release v1.32 milestone Aug 29, 2018

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