Batch code refactoring #49818
Batch code refactoring #49818
Conversation
salt/cli/batch.py
Outdated
| if 'password' in kwargs: | ||
| eauth['password'] = kwargs.pop('password') | ||
| if 'token' in kwargs: | ||
| eauth['token'] = kwargs.pop('token') |
There was a problem hiding this comment.
I know you did not wrote this thing and just moved, but let's help it here:
for opt in ['eauth', 'username', 'password', 'token']:
if opt in kwargs:
eauth[opt] = kwargs.pop(opt)
salt/cli/batch.py
Outdated
| if 'gather_job_timeout' in kwargs: | ||
| opts['gather_job_timeout'] = kwargs['gather_job_timeout'] | ||
| if 'batch_wait' in kwargs: | ||
| opts['batch_wait'] = int(kwargs['batch_wait']) |
There was a problem hiding this comment.
I am wondering why batch_wait needed to be int()'ed here. I'd rather add it to the config, same as timeout etc.
There was a problem hiding this comment.
And also shrink this too in for opt... loop, rather three time if.
There was a problem hiding this comment.
@isbm this could all be good ideas but I don't want to incorporate them at this point because I'm trying to do a refactoring here. The refactoring has already a high chance of introducing you bugs and adding additional changes would raise it even more.
Let's try to keep all these ideas for after I am done with the refactoring and let's do them in the following iteration. Agreed?
There was a problem hiding this comment.
Ah, WP, right. Yeah, we can make it better with the follow-up then.
|
Hi @dincamihai Just waiting on one response to feedback from you here and then I think we can move forward with this. Thanks! |
|
@dincamihai Did you have a chance to come back to this? |
|
@rallytime I'm working on it. I'll try to respond the questions above now. |
d4b28f5 to
9a5513a
Compare
|
@rallytime just a note |
|
@dincamihai I think it's nice to iterate on smaller things when we can, so if it's alright with you, let's get this in and you can submit your other work at a later date. |
|
I think the last thing we need before we can get this in is an addition to the release notes. @dincamihai can you take care of that? |
|
hello @cachedout sorry for the late reply. By the way, I have prepared the second part of refactorings: I have not created a PR yet because I'm not sure how to approach it given it is so big. |
003fa92 to
3b38620
Compare
3b38620 to
73fe362
Compare
|
@cachedout I have pushed those commits as part of this PR since they build on top of the other already included (and not yet merged) commits. |
|
After the tests run this should be ready to go. |
|
@dincamihai This needs to be rebased to the |
|
@dincamihai merges to Salt are also now requiring tests. Are you still interested in driving this to completion? |
|
@dincamihai do you need any help with this? Are you still planning to drive it to completion? |
|
@dincamihai looks like we have asked you rebase this against master and added tests. We have not heard from you in some time. We may consider this PR abandoned and close it soon. |
|
Reviewed in Grooming 2021-APR-05 considering closing |
|
can you please rebase against the master branch? we are no longer taking changes against develop. Discussed in Grooming 2021-APR-19 have requested rebase with no response, closing. |
What does this PR do?
This started from RFC#0002 but I realized later that it is not required for the implementation of the RFC.
Another PR will address the implementation.
This PR is a gradual refactoring.
For the reviewer: It is easier to review the PR commit by commit to understand how I got to the end result. Thanks!
Tests written?
Not yet