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

Features/vault enterprise #58586

Merged
merged 57 commits into from Apr 29, 2021
Merged

Conversation

eadderley-tc
Copy link
Contributor

@eadderley-tc eadderley-tc commented Sep 30, 2020

What does this PR do?

This PR enables Salt to communicate with an Enterprise Vault via X-Vault-Namespace headers.

What issues does this PR fix or reference?

Fixes: 58585

Merge requirements satisfied?

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

Not entirely sure how to write tests for this, as the headers don't break anything if no namespace is supplied and Vault doesn't complain if namespaces aren't enabled.

Commits signed with GPG?

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.

@eadderley-tc eadderley-tc requested a review from a team as a code owner September 30, 2020 13:35
@eadderley-tc eadderley-tc requested review from dwoz and removed request for a team September 30, 2020 13:35
@welcome
Copy link

welcome bot commented Sep 30, 2020

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at core@saltstack.com or reach out directly to the Community Manager, Cassandra Faris via Slack. We’re glad you’ve joined our community and look forward to doing awesome things with you!

salt/utils/vault.py Outdated Show resolved Hide resolved
salt/utils/vault.py Outdated Show resolved Hide resolved
salt/utils/vault.py Show resolved Hide resolved
salt/utils/vault.py Outdated Show resolved Hide resolved
@DmitryKuzmenko
Copy link
Contributor

@eadderley-tc thank you for PR. I've put some comments to my review.
Also about tests: in this case it's needed to add or update the existing tests to cover your changes. I.e. check that the code works as expected in all the cases you've added: there's no namespace specified, there's a namespace specified, exception cases.

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

@DmitryKuzmenko We've added some tests and cleaned up the linting. Sorry about the delay, got pulled off for project work and illness. Can you explain what's going wrong in that ci/pre-commit?

@sagetherage
Copy link
Contributor

@dwoz or @s0undt3ch can you review this PR please? @eadderley-tc Dmitry is unfortunately no longer on the project, but looking to get this reviewed. Pre-commit can be pain, if it is helpful please attend or get with @waynew on IRC, Community Slack or at the Test Clinics we hold on Tue/Thu via Twitch.

Copy link
Member

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

Please remove the changes under doc/man those get generated automatically.

And there's also conflicts that need to be solved.

@eadderley-tc
Copy link
Contributor Author

Please remove the changes under doc/man those get generated automatically.

What do they get automatically generated from?

@vveliev-tc
Copy link
Contributor

@s0undt3ch not sure why some tests are failing, what is currently outstanding for this PR?

@vveliev-tc
Copy link
Contributor

re-run centos7-py3-pycryptodome-pytest

@s0undt3ch
Copy link
Member

Can you please rebase against the latest master branch changes? Thanks!

@vveliev-tc
Copy link
Contributor

re-run windows2016-py3-pytest

@vveliev-tc
Copy link
Contributor

re-run windows2019-py3-pytest

@vveliev-tc
Copy link
Contributor

s0undt3ch - is there anything we should do about failing tests?

@eadderley-tc
Copy link
Contributor Author

@s0undt3ch As far as we can tell (asked on the Slack too) the remaining failing tests are broken. Can we get another review?

s0undt3ch
s0undt3ch previously approved these changes Apr 1, 2021
@eadderley-tc
Copy link
Contributor Author

@s0undt3ch Latest master commits merged in

@eadderley-tc
Copy link
Contributor Author

@s0undt3ch What are out next steps here? Eager to get this merged.

salt/modules/vault.py Show resolved Hide resolved
Co-authored-by: Megan Wilhite <megan.wilhite@gmail.com>
Ch3LL
Ch3LL previously approved these changes Apr 21, 2021
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

Looks like there is a just one last lint failure, probably caused by my suggestion. Apologies on that.

@Ch3LL Ch3LL requested a review from s0undt3ch April 29, 2021 10:31
@Ch3LL Ch3LL merged commit 56911b6 into saltstack:master Apr 29, 2021
@welcome
Copy link

welcome bot commented Apr 29, 2021

Congratulations on your first PR being merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vault Enterprise Integration [FEATURE REQUEST]
9 participants