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

fix TypeError: argument of type int is not iterable #52624

Merged
merged 11 commits into from Apr 29, 2019

Conversation

@tanlingyun2005
Copy link
Contributor

commented Apr 19, 2019

What does this PR do?

fix a batch run bug, when batch_safe_size is set to a integer or percentage, will resulting in "TypeError: argument of type 'int' is not iterable"

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@tanlingyun2005 tanlingyun2005 requested a review from saltstack/team-core as a code owner Apr 19, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@tanlingyun2005 thanks for the contribution! Looks like there are some test failures:

    unit.cli.test_batch.BatchTestCase.test_get_bnum_high_percentage
    unit.cli.test_batch.BatchTestCase.test_get_bnum_percentage

Also is it possible for you to write a regression test for this fix?

@tanlingyun2005

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@tanlingyun2005 thanks for the contribution! Looks like there are some test failures:

    unit.cli.test_batch.BatchTestCase.test_get_bnum_high_percentage
    unit.cli.test_batch.BatchTestCase.test_get_bnum_percentage

Also is it possible for you to write a regression test for this fix?

OK,Let me see

@tanlingyun2005

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

jenkins/pr/lint — Python lint test has failed

Incompatible Python 3 code found. Proposed fix:

because unicode type just exists in Python 2, so "isinstance(self.opts['batch'], unicode)" is not compatible to Python 3, but use "sys.version_info.major" as a conditional branch could avoid this question.

@waynew

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@tanlingyun2005 there's some samples around the code of checking for bytes vs text using six - you'll probably want to follow one of those patterns.

dextertan and others added 2 commits Apr 23, 2019
@tanlingyun2005

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@tanlingyun2005 there's some samples around the code of checking for bytes vs text using six - you'll probably want to follow one of those patterns.

Thanks remind

@Akm0d
Akm0d approved these changes Apr 23, 2019
Copy link
Contributor

left a comment

Looks good

Copy link
Contributor

left a comment

would you mind writing a regression test for this so this issue does not pop up again? thanks :)

@tanlingyun2005

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

would you mind writing a regression test for this so this issue does not pop up again? thanks :)

sorry, I don't know how to write "regression test", could you give me some samples?
but I add a unit test case to tests/unit/cli/test_batch.py, this is a run result:
test_get_bnum_high_percentage (main.BatchTestCase)
[CPU:13.3%|MEM:41.5%] ... ok
test_get_bnum_int (main.BatchTestCase)
[CPU:0.0%|MEM:41.5%] ... ok
test_get_bnum_invalid_batch_data (main.BatchTestCase)
[CPU:25.0%|MEM:41.5%] ... <type 'unicode'>
ok
test_get_bnum_percentage (main.BatchTestCase)
[CPU:0.0%|MEM:41.5%] ... ok
test_get_bnum_str (main.BatchTestCase)
[CPU:0.0%|MEM:41.5%] ... ok


Ran 5 tests in 0.009s

OK

@dwoz
dwoz approved these changes Apr 27, 2019
Copy link
Contributor

left a comment

@tanlingyun2005

A 'regression test' is basically a tests that shows the bug when the tests fails. Generally, we write a regression test first before fixing the bug in question. The purpose of these types of tests is to make sure that once a bug is fixed it does not happen again in the future.

Your unit test looks good for this case. Thanks a ton!

@@ -85,7 +85,7 @@ def get_bnum(self):
'''
partition = lambda x: float(x) / 100.0 * len(self.minions)
try:
if '%' in self.opts['batch']:
if isinstance(self.opts['batch'], six.string_types) and '%' in self.opts['batch']:

This comment has been minimized.

Copy link
@angrydead

angrydead Apr 27, 2019

Love the six.string_types for Python 2 and 3 compatibility.

@Ch3LL
Ch3LL approved these changes Apr 29, 2019
@Ch3LL Ch3LL merged commit 9a1ed78 into saltstack:2019.2.1 Apr 29, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
@tanlingyun2005 tanlingyun2005 deleted the tanlingyun2005:2019.2.1 branch Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.