Update pcs support for > 0.10 - #60257
Conversation
garethgreenaway
added
the
needs-testcase
s0undt3ch
left a comment
s0undt3ch
left a comment
There was a problem hiding this comment.
Now all we need is tests covering your changes....
Problem is that there is no existing test on that module/state. If they existed I would have updated them. I don't have any idea about how to do this. |
|
@s0undt3ch How do we resolve this? Can we skip the tests or can someone start with the tests and I just add to it? |
|
I'll write a couple tests for you so you can start form there and I'll push it to your branch. I'm working on them now. |
|
@Ch3LL Thank you! |
|
I pushed some pcs module tests :) |
Ch3LL
left a comment
Ch3LL
left a comment
There was a problem hiding this comment.
Just one more change request from me
| return ret | ||
| if not isinstance(extra_args, (list, tuple)): | ||
| extra_args = [] | ||
| if "--force" not in extra_args: |
There was a problem hiding this comment.
I'm good with that then, but can we add back the ability to pass in extra_args.
|
ping @garethgreenaway would you also mind reviewing here as well? |
|
With the change that @Ch3LL requested for |
|
just a couple of test failures that need to be cleaned up here and then we can get this merged :) |
|
Updated. There is some documentation fail but I can't work out what's going on. |
|
Is there anything still outstanding on this? |
|
pre-commit is failing and needs to be cleaned up |
|
Looks successful? If not. Please assist. I can't see how to fix any of the transient errors that have popped up. |
|
Congratulations on your first PR being merged! 🎉 |
|
Thanks :)
…On Tue, 24 Aug 2021, 20:04 welcome[bot], ***@***.***> wrote:
Congratulations on your first PR being merged! 🎉
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#60257 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFORIRJCHCWBHRX7NSDTALT6PNK3ANCNFSM45RVTNKQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
* PCS module and state updated to handle pcs version > 0.10 * PCS module and state updated to handle pcs version > 0.10 - changelog * Removed six * Module documentation updated * Requested changes implemented * Add pcs module tests * Tests added and is_auth altered to fit purpose * Tests added. Debian bypass for broken config creation bypassed. * Tests debug output removed * Minor linting fixes * Update salt/modules/pcs.py Co-authored-by: Megan Wilhite <megan.wilhite@gmail.com> * Updated as per @Ch3LL's recommendations and other pre-commit fails * Removed test_config_show and some commented out print lines * Extra documentation added to cluster_setup * Updated item_show to work properly with properties and updated tests to match * Fixed change requests and added a wipe_default flag to remove default config * Extra args now gets passed from the pcs state to the auth function in the pcs module. * Minor formatting fix * Minor build fix to make linter happy Co-authored-by: Wayne Gemmell <wayne@connect-mobile.co.za> Co-authored-by: Megan Wilhite <megan.wilhite@gmail.com> Co-authored-by: Sage the Rage <36676171+sagetherage@users.noreply.github.com>
6 participants
What does this PR do?
This PR accommodates the cluster auth and setup when using newer version of PCS.
What issues does this PR fix or reference?
Fixes: #56924
Previous Behavior
When authenticating and setting up a cluster the command would fail against pcs < 0.10
New Behavior
Creating and authenticating the cluster is successful.
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
No