add new flag: --skip-service #276

tamsky · 2017-06-05T01:39:20Z

tox -e docs was producing non-useful diffs.
Please suggest a better test?
I'll be the first to admit, the method used to disable services is slightly unconventional, but I believe the approach employed results in the least number of changed lines of code.

Contributor License Agreement

By submitting this work for inclusion in awslimitchecker, I agree to the following terms:

The contribution included in this request (and any subsequent revisions or versions of it)
is being made under the same license as the awslimitchecker project (the Affero GPL v3,
or any subsequent version of that license if adopted by awslimitchecker).
My contribution may perpetually be included in and distributed with awslimitchecker; submitting
this pull request grants a perpetual, global, unlimited license for it to be used and distributed
under the terms of awslimitchecker's license.
I have the legal power and rights to agree to these terms.

Signed-off-by: tamsky

tamsky · 2017-06-05T01:40:10Z

Related feature request: #275

jantman · 2017-06-05T10:09:16Z

@tamsky Thank you very much for this; this will also help the person who opened #267.

This is a bit more complicated of a PR, so I'm going to need a bit longer to review it. The TravisCI tests also failed on all unit test runs, so I can't see coverage information... but it looks like the failures were all due to pep8 violations (specifically, lines 105 and 131 in runner.py being longer than 80 characters).

The docs build failure was happening at the time of your last PR as well. From what I can tell, it is most likely related to an issue or change in the build of the official Python docs. I'll ignore those failures for now, and fix the problem on develop.

tamsky · 2017-06-05T17:59:15Z

TravisCI tests all fixed except one, which I'll guess you're better equipped to diagnose.

jantman · 2017-06-05T18:23:52Z

Thanks. That's the docs build failure that I mentioned in my last comment. It's not related to your changes, so I'll merge this to develop and sort it out sometime this week.

It looks like code coverage decreased a bit on this; there are a few branches that aren't covered. I can fix those up after merge though.

I'm going to go through the code now.

jantman

The idea behind this is great, but there are some stylistic changes that could greatly simplify the code. I can merge this if you make them, or I can pull in these changes and rework them myself before I cut a new release later this week.

jantman · 2017-06-05T18:26:06Z

awslimitchecker/checker.py

+          returning limits for
+        :type service_to_skip: list
+        """
+        if service_to_skip is not None:


The code and signature of this function would be much simpler of service_to_skip defaulted to an empty list instead of None. That would allow removal of the if branch here, and make the signature of the function look more like what it does. Also, service in the variable name should be pluralized.

Also, for what it's worth, I'd probably rename this function to either remove_services or skip_services.

function signature updated as suggested.
if removed.
service_to_skip variable name changed to services_to_remove.

jantman · 2017-06-05T18:42:01Z

awslimitchecker/runner.py

@@ -358,6 +361,11 @@ def console_entry_point(self):
            ta_refresh_timeout=args.ta_refresh_timeout
        )

+        if args.skip_service is not None:
+            if args.service is not None:
+                raise SystemExit(0)


This conditional needs a bit more work. If we're considering skip_service and service to be mutually exclusive, then that should be handled in parse_args, by telling Argparse that the options are mutually exclusive (see line 164 for an example of adding a mutually exclusive argument group).

Aside from that, on these lines we're raising a SystemExit with code 0, which implies success, but this is actually an error condition. That would be very confusing to a user, as the program would exit early with no indication of why.

I actually implemented the add_mutually_exclusive_group() style arg, but wound up without a firm belief that they were, in fact, mutually exclusive.

If someone wanted to list --service A B C and also --skip-service C, I can't say it's strictly wrong, or even confusing.

Happy to remove this block (364-366) if you agree.

Yeah, I agree. I think it's clear enough.

Aside from this one last bit here, your last commit looks good; I'll merge once these conditionals are removed and tests pass.

jantman · 2017-06-05T18:42:59Z

awslimitchecker/runner.py

@@ -358,6 +361,11 @@ def console_entry_point(self):
            ta_refresh_timeout=args.ta_refresh_timeout
        )

+        if args.skip_service is not None:


This conditional is not needed at all; the add_argument call for --skip-service can have default=[] added to it to produce an empty list if not specified; that can be passed directly in to remove_skipped_services.

codecov-io · 2017-06-05T19:39:29Z

Codecov Report

Merging #276 into develop will decrease coverage by 0.11%.
The diff coverage is 75%.

@@             Coverage Diff             @@
##           develop     #276      +/-   ##
===========================================
- Coverage      100%   99.88%   -0.12%     
===========================================
  Files           23       23              
  Lines         1691     1699       +8     
  Branches       262      265       +3     
===========================================
+ Hits          1691     1697       +6     
- Misses           0        1       +1     
- Partials         0        1       +1

Impacted Files	Coverage Δ
awslimitchecker/checker.py	`100% <100%> (ø)`	⬆️
awslimitchecker/runner.py	`98.91% <60%> (-1.09%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57466c7...a6b0bf5. Read the comment docs.

tamsky · 2017-06-05T19:47:02Z

awslimitchecker/tests/test_runner.py

@@ -614,6 +619,19 @@ def test_entry_no_service_name(self, capsys):
        assert excinfo.value.code == 6
        assert self.cls.service_name is None

+    def test_entry_no_service_name_skip_service(self, capsys):


My question regarding tests was here.

Can we perform a better test than this?

We can, but you've done more than enough work on this PR... that's entirely up to you. The test suite is pretty clunky, so I usually expect to add or rework some tests after merging PRs.

Ok, well then -- thank you for the codereview, and suggested edits, they were all spot on.
I've definitely learned a few things along the way here.

I'll watch what you do to improve this test from the sidelines.

Ok. It's probably not going to be until this weekend, sorry.

add new flag: --skip-service

deaec7f

tamsky added 2 commits June 5, 2017 10:47

issue/275: fix pip8 violations

bb0f815

issue/275: fix pep8 violation

808e4a2

jantman requested changes Jun 5, 2017

View reviewed changes

issue/275: implement suggested style changes

2237174

tamsky added 3 commits June 5, 2017 12:42

issue/275: add default value to arg parser

2875fda

issue/275: remove check against services & skip_services both being set

b40877f

issue/275: add default value to test arg parser

a6b0bf5

tamsky commented Jun 5, 2017

View reviewed changes

jantman merged commit 1eb4672 into jantman:develop Jun 7, 2017

tamsky deleted the issue/275 branch June 7, 2017 23:07

jantman added a commit that referenced this pull request Jun 11, 2017

PR #276 - fixup tests for AwsLimitChecker.remove_services

4900195

jantman added a commit that referenced this pull request Jun 11, 2017

PR #276 - actually implement the --skip-service option

9e55bd4

jantman added a commit that referenced this pull request Jun 11, 2017

PR #276 - tests for --skip-service

b66ac3a

jantman added a commit that referenced this pull request Jun 11, 2017

PR #276 document remove_services()

228e79a

jantman added a commit that referenced this pull request Jun 11, 2017

fixup docs for PR #276

ee7135b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new flag: --skip-service #276

add new flag: --skip-service #276

tamsky commented Jun 5, 2017 •

edited

Loading

tamsky commented Jun 5, 2017

jantman commented Jun 5, 2017

tamsky commented Jun 5, 2017

jantman commented Jun 5, 2017

jantman left a comment

jantman Jun 5, 2017

jantman Jun 5, 2017

tamsky Jun 5, 2017

jantman Jun 5, 2017

tamsky Jun 5, 2017

jantman Jun 5, 2017

jantman Jun 5, 2017

tamsky Jun 5, 2017

jantman Jun 5, 2017

tamsky Jun 5, 2017

codecov-io commented Jun 5, 2017 •

edited

Loading

tamsky Jun 5, 2017

jantman Jun 5, 2017

tamsky Jun 5, 2017

jantman Jun 7, 2017

add new flag: --skip-service #276

add new flag: --skip-service #276

Conversation

tamsky commented Jun 5, 2017 • edited Loading

Contributor License Agreement

tamsky commented Jun 5, 2017

jantman commented Jun 5, 2017

tamsky commented Jun 5, 2017

jantman commented Jun 5, 2017

jantman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 5, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamsky commented Jun 5, 2017 •

edited

Loading

codecov-io commented Jun 5, 2017 •

edited

Loading