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.

Sign up for GitHub

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

Jump to bottom

(12.1.0) INFRASYS-6779 - disable the cloud-watch_alarms while rolling deployment. #27

Merged
merged 2 commits into from
Apr 4, 2016
Merged

(12.1.0) INFRASYS-6779 - disable the cloud-watch_alarms while rolling deployment. #27

merged 2 commits into from
Apr 4, 2016

Conversation

pthatchinamoorthy
Copy link
Contributor

Disabled the alarms before deployment
Enabled the alarms before deployment

@pthatchinamoorthy pthatchinamoorthy force-pushed the INFRASYS-6779 branch 4 times, most recently from 801c332 to 82cdfae Compare March 7, 2016 04:18
@pthatchinamoorthy
Copy link
Contributor Author

closing the PR and opening it again to re-run the travis build.

@pthatchinamoorthy pthatchinamoorthy changed the title Infrasys 6779 Infrasys 6779 - disable the cloud-watch_alarms while rolling deployment. Mar 7, 2016
@adamjkeller
Copy link
Contributor

@prabhakar1983 can you do me a favor and move the README/additional test configurations (7fda27e ) out of this pull request? I'd prefer this PR just focus on cloud_watch alarm changes

@pthatchinamoorthy
Copy link
Contributor Author

@adamjkeller Sure. will ping you once it is done.

So, you need this PR to do only cloud_watch_disable_changes and fix for test cases, correct ?

@adamjkeller
Copy link
Contributor

@prabhakar1983 correct, thank you!

@pthatchinamoorthy
Copy link
Contributor Author

@adamjkeller Removed the commit that has the documentation. can you pelase review. thank you

adamjkeller
adamjkeller reviewed Mar 8, 2016
View reviewed changes
License2Deploy/AWSConn.py
@@ -31,6 +32,13 @@ def aws_conn_elb(region, profile='default'):
return conn
except Exception as e:
logging.error("Unable to connect to region, please investigate: {0}".format(e))
@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a newline above the start of this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamjkeller Done.

@pthatchinamoorthy pthatchinamoorthy force-pushed the INFRASYS-6779 branch 5 times, most recently from fefcb06 to e8ba8d8 Compare March 9, 2016 00:13
@pthatchinamoorthy
Copy link
Contributor Author

@adamjkeller added Lamda function. Thank you

@pthatchinamoorthy pthatchinamoorthy force-pushed the INFRASYS-6779 branch 2 times, most recently from 48b555a to ac30b03 Compare March 9, 2016 05:04
@pthatchinamoorthy
Copy link
Contributor Author

@adamjkeller Added test cases.

adamjkeller
adamjkeller reviewed Mar 9, 2016
View reviewed changes
tests/rolling_deploy_test.py
def test_cloudwatch_filter_function_with_no_alarm(self):
bad_alarm = MetricAlarm(name="xyz_CloudWatchAlarm", metric="CPUUtilization", namespace="AWS/EC2")
cloud_watch_alarms = filter(lambda alarm : self.rolling_deploy.project in alarm.name and self.rolling_deploy.env in alarm.name, [ bad_alarm])
print("alarms size {}".format(len(cloud_watch_alarms)))
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to print here or in the method below.

And in python 2.6, you need to add the number from the index into the {}.

So you should do: print("alarms size {0}".format(len(cloud_watch_alarms)))

@pthatchinamoorthy pthatchinamoorthy force-pushed the INFRASYS-6779 branch 2 times, most recently from 1e459e4 to b6be111 Compare March 26, 2016 16:05
@coveralls
Copy link

Coverage Status

Coverage decreased (-59.6%) to 35.2% when pulling b6be111 on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@pthatchinamoorthy pthatchinamoorthy force-pushed the INFRASYS-6779 branch 3 times, most recently from 1cba4ae to ba5aac7 Compare March 26, 2016 16:39
@coveralls
Copy link

Coverage Status

Coverage decreased (-11.6%) to 83.2% when pulling ba5aac7 on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-52.4%) to 42.4% when pulling 168746b on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 94.8% when pulling 1a2bcff on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@pthatchinamoorthy pthatchinamoorthy force-pushed the INFRASYS-6779 branch 2 times, most recently from e95616c to 5094bc4 Compare March 27, 2016 03:27
@coveralls
Copy link

Coverage Status

Coverage decreased (-52.4%) to 42.4% when pulling 5094bc4 on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-52.4%) to 42.4% when pulling 5094bc4 on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.4%) to 90.4% when pulling b6d5cc2 on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 94.8% when pulling a766be3 on prabhakar1983:INFRASYS-6779 into bfff745 on dandb:master.

@pthatchinamoorthy
Copy link
Contributor Author

@adamjkeller @sbraverman Increased the tests coverage. Can you please review. thank you.

@adamjkeller
Copy link
Contributor

At this moment, the unit tests are covering the exception in the new methods, but not the success. The reason behind this is that disable_alarm and enable_alarm actions have not been implemented in moto.

I would prefer to see particular method mocked out so are able to test a successful run along with a non successful run of the method.

@adamjkeller
Copy link
Contributor

Ticket created to mock out unit tests properly:

INFRASYS-7023

@adamjkeller
Copy link
Contributor

+1

@kantselovich kantselovich changed the title Infrasys 6779 - disable the cloud-watch_alarms while rolling deployment. INFRASYS-6779 - disable the cloud-watch_alarms while rolling deployment. Mar 30, 2016
sbraverman
sbraverman reviewed Mar 30, 2016
View reviewed changes
License2Deploy/rolling_deploy.py
logging.error("Error while retrieving the list of cloud-watch alarms. Error: {0}".format(e))
exit(self.exit_error_code)
project_cloud_watch_alarms = filter(lambda alarm: self.project in alarm.name and self.env in alarm.name, all_cloud_watch_alarms)
if len(project_cloud_watch_alarms) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

it is enough to do:

if not project_cloud_watch_alarms:

@sbraverman
Copy link
Contributor

+1

@taoistmath
Copy link
Contributor

1+

@adamjkeller adamjkeller changed the title INFRASYS-6779 - disable the cloud-watch_alarms while rolling deployment. (12.1.0) INFRASYS-6779 - disable the cloud-watch_alarms while rolling deployment. Mar 30, 2016
@adamjkeller adamjkeller merged commit 64ff3cc into dandb:master Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pthatchinamoorthy @adamjkeller @coveralls @sbraverman @taoistmath