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

Enhancement: Allowing alarms to be disabled in stages #205

Merged
merged 14 commits into from
Jan 17, 2023

Conversation

malachi-constant
Copy link
Contributor

@malachi-constant malachi-constant commented Jan 11, 2023

Enhancement

  • Adds ability to disable cloudwatch alarms in DataStages by setting alarm_threshold value to 0
  • Adds test using the above setting

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@malachi-constant malachi-constant self-assigned this Jan 11, 2023
@malachi-constant malachi-constant marked this pull request as draft January 11, 2023 19:02
@malachi-constant malachi-constant added enhancement New feature or request core labels Jan 11, 2023
@malachi-constant malachi-constant added this to In Progress in Roadmap Jan 11, 2023
@malachi-constant malachi-constant moved this from In Progress to In Review in Roadmap Jan 11, 2023
@malachi-constant malachi-constant marked this pull request as ready for review January 11, 2023 21:40
Copy link
Contributor

@LeonLuttenberger LeonLuttenberger left a comment

Choose a reason for hiding this comment

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

Just an idea, but I wonder if it would be better to just have a standard alarms_enabled parameter that's define in Stage, and is therefore inherited by all the stages.

I'm just thinking it might be easier if all the stages have the same parameter which can be used to disable the alarm, as opposed to each stage having it be a different name.

@malachi-constant
Copy link
Contributor Author

Just an idea, but I wonder if it would be better to just have a standard alarms_enabled parameter that's define in Stage, and is therefore inherited by all the stages.

I'm just thinking it might be easier if all the stages have the same parameter which can be used to disable the alarm, as opposed to each stage having it be a different name.

Yep that's kind of what I was thinking with the alarm_threshold=0 behavior but it having its own explicit parameter would be best. The only drawback I can see with that is future cases could involve multiple alarms where the user might want to be selective about what is created.

@malachi-constant malachi-constant marked this pull request as draft January 14, 2023 00:22
@malachi-constant malachi-constant marked this pull request as ready for review January 14, 2023 00:33
@malachi-constant malachi-constant moved this from In Review to In Progress in Roadmap Jan 17, 2023
@malachi-constant malachi-constant merged commit 54acff9 into main Jan 17, 2023
@malachi-constant malachi-constant deleted the enhancement/allow-disable-of-alarms branch January 17, 2023 15:37
@malachi-constant malachi-constant moved this from In Progress to Done in Roadmap Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants