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

Port #49480 to master #56830

Closed

Conversation

DmitryKuzmenko
Copy link
Contributor

Port #49480 to master

Salt-based supportconfig. Re-implementation of Support Config Tool in
Salt.

@DmitryKuzmenko DmitryKuzmenko requested a review from a team as a code owner April 22, 2020 17:05
@ghost ghost requested review from dwoz and removed request for a team April 22, 2020 17:05
Bo Maryniuk and others added 2 commits April 28, 2020 14:56
Salt-based supportconfig. Re-implementation of Support Config Tool in
Salt.
@DmitryKuzmenko DmitryKuzmenko marked this pull request as draft August 31, 2020 12:21
os.access documentation notes it could be false-positive in some cases
like network filesystems. So we have to catch the permission error
anyway that makes using of os.access check redundant.
@DmitryKuzmenko DmitryKuzmenko marked this pull request as ready for review August 31, 2020 12:38
@DmitryKuzmenko DmitryKuzmenko marked this pull request as draft September 1, 2020 17:21
self.out.error(ret)
except Exception as ex: # pylint: disable=broad-except
ret = "Unhandled exception occurred: {}".format(ex)
log.debug(ex, exc_info=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ret? And not ex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose is to collect all events so it just puts the error as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but the above ret is not used. I guess it was supposed to be on the log call, of not, nuke it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I got you. Sorry.

salt/cli/support/collector.py Show resolved Hide resolved
tests/pytests/unit/cli/test_support.py Outdated Show resolved Hide resolved
tests/pytests/unit/cli/test_support.py Outdated Show resolved Hide resolved
@DmitryKuzmenko DmitryKuzmenko marked this pull request as ready for review September 2, 2020 15:12
@DmitryKuzmenko
Copy link
Contributor Author

@isbm could you please take a look at this change I've made in your original code 15e48c2 ?

@dwoz dwoz added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Sep 22, 2020
@DmitryKuzmenko
Copy link
Contributor Author

Superseded by #57054

@DmitryKuzmenko DmitryKuzmenko deleted the master-port-49480 branch October 23, 2020 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master-port Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants