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 #52305 to master #56785

Merged
merged 12 commits into from
May 7, 2020
Merged

Port #52305 to master #56785

merged 12 commits into from
May 7, 2020

Conversation

xeacott
Copy link
Contributor

@xeacott xeacott commented Apr 21, 2020

What does this PR do?

What issues does this PR fix or reference?

Fixes:

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

  • Docs
  • Changelog
  • Tests written/updated

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

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

@xeacott xeacott requested a review from a team as a code owner April 21, 2020 18:21
@ghost ghost requested review from cmcmarrow and removed request for a team April 21, 2020 18:21
@dwoz
Copy link
Contributor

dwoz commented Apr 21, 2020

@xeacott Test failures look legit

@xeacott xeacott changed the title Port #52305 to master DO NOT MERGE Port #52305 to master Apr 24, 2020
@xeacott xeacott changed the title DO NOT MERGE Port #52305 to master Port #52305 to master Apr 30, 2020
@@ -46,8 +47,6 @@
)
from salt.utils.openstack.swift import SaltSwift

# pylint: enable=no-name-in-module,import-error
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not down with removing these for the whole file.

@@ -551,7 +571,7 @@ def s3_opt(key, default=None):
https_enable=s3_opt("https_enable", True),
)
return dest
except Exception as exc: # pylint: disable=broad-except
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be changing unless we are actually going to put a more applicable exception type here (which would be the preference).

@xeacott
Copy link
Contributor Author

xeacott commented May 4, 2020

Thanks for reviewing, I'll add them back. This PR was very troublesome not sure how all the extra changes weren't caught by the linter...

@cmcmarrow cmcmarrow requested a review from dwoz May 6, 2020 22:21
@dwoz dwoz merged commit 8c6eaea into saltstack:master May 7, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants