-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add support for policy objects #324
Conversation
a8e832e
to
58bacae
Compare
58bacae
to
9960ab2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this pull-request, and it is functional. Only minor issues that I could work around. Good how you made it fit in nicely with the rest of the module.
Thank you for your review! I will work on the issues you spotted but it might take a few weeks as i've dug down in some other projects... |
04dcd05
to
6d57ba6
Compare
As you might have seen i've addressed the issues you spotted but i won't have time to test them properly until some time in august at best. |
Only exclude catalog rules applied to the current zone from purging.
d98491f
to
dbbca6a
Compare
I've squished my fixes and rebased on master to remove the merge conflict. After rebasing the spec tests report an error on my workstation but i don't think it's related as the same error is present in the master branch. |
Sorry, i need to fix another thing. The firewalld_policy type insists that ingress and egress zones are arrays which is normally resonable but i don't think it is when ensure is absent. |
74f2bcc
to
15e2c23
Compare
There, ingress_zone and egress_zone may now be emtpy if ensure is absent. |
Can I help get this ready for merge in? |
@qha can you squash this down again? |
Sorry for being so slow to respond, i was busy going to and recovering from a conference. |
end | ||
end | ||
|
||
def purge_rich_rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would adding the #instances method to the provider(s) be an alternative to this to allow purging of unmanaged resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps i would have known while this was fresh in my mind but i don't now. I hope someone else can answer this.
The main squashing I had in mind was regarding the readme and reference updates. Having the parameter name tweaks in a separate commit may or may not add clarity. |
I see. If i recall correctly i did it that way because the reference had not been updated the last few commits so updating it introduced unrelated changes that i did not want to put in the same commit. |
That is fine with me |
Closes: voxpupuli#316 (Also recreate reference with 'bundle exec rake reference'.)
15e2c23
to
708ff6c
Compare
I can't find the other changes i thought i remembered so i just squashed the last commit into the main one. |
Unless there are any objections I'd like to merge this in next week. |
Fixes: #316