Skip to content

Created a script to enable/disable a list of alerts #72

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Jan 25, 2019

Conversation

KranzSysdig
Copy link
Contributor

Cloned the 'update_alerts.py' example to create a script that would take a list of alerts, and flip their enabled status. A customer had requested the ability to selectively enable / disable a list of alerts on a schedule, so I created this so they could run it on a cronjob. Will also add this as a FR.

KranzSysdig and others added 2 commits January 25, 2019 11:47
Cloned the 'update_alerts.py' example to create a script that would take a list of alerts, and flip their enabled status. A customer had requested the ability to selectively enable / disable a list of alerts on a schedule, so I created this so they could run it on a cronjob. Will also add this as a FR.
@davideschiera davideschiera merged commit eb07cff into master Jan 25, 2019
@davideschiera davideschiera deleted the flip_alerts branch January 25, 2019 22:16
if alert['enabled'] == True:
alert['enabled'] = False
else:
alert['enabled'] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this is because copy-pasting old code. But this could be improved by a single alert['enabled'] = not alert['enabled']

@davideschiera Should we step by step improve this repo code when adding new code?

Copy link
Contributor

Choose a reason for hiding this comment

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

The one-liner seems reasonable (especially in this context, as long as you can "read" the line and it sounds clear enough, then it's all good).

In terms of refactoring, I think it's a good idea. I actually missed this one when I reviewed the PR last week, sorry about that.

Feel free to file PRs to clean up the code base, we can also discuss offline about the major points to solve. Thanks!!

Copy link
Contributor

Choose a reason for hiding this comment

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

@davideschiera If not in a hurry, you can let another pair of 👀 to take a look at the PR 😄 By now, we can live without this one-liner.

Don't have the desired time to attend this repo, but 👍 to offline discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Thanks for bringing it up!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants