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

master: firewall: add --use-system-defaults arg to command #203

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Projects
None yet
4 participants
@dustymabe
Contributor

dustymabe commented Dec 15, 2017

Needed for [1] where we would like to include firewalld
and configure firewalld in Atomic Host (in the ostree) and
have Anaconda leave the delivered "defaults" in place.

[1] https://pagure.io/atomic-wg/issue/401

@dustymabe dustymabe changed the title from firewall: add --use-system-defaults arg to command to master: firewall: add --use-system-defaults arg to command Dec 15, 2017

@dustymabe dustymabe force-pushed the dustymabe:master branch from 1b58bbb to 0134ac2 Dec 15, 2017

def _getParser(self):
op = F14_Firewall._getParser(self)
op.add_argument("--remove-service", dest="remove_services", help="",
action=ExtendAction, type=commaSplit, version=F20)
op.add_argument("--use-system-defaults", dest="use_system_defaults",
version=F20, help="""

This comment has been minimized.

@cgwalters

cgwalters Dec 15, 2017

Contributor

Shouldn't this have a higher version=?

This comment has been minimized.

@dustymabe

dustymabe Dec 15, 2017

Contributor

I was mostly matching the --remove-service option that was already there. Depending on this review I might have to make a whole new subclass for F28, but I don't know what the convention is.

This comment has been minimized.

@clumens

clumens Dec 15, 2017

Collaborator

This is probably a little confusing and could be better documented... somewhere. In this context, the version= parameter is used to generate documentation. For your new option, you would use version=F28 (or whatever).

The general idea is that the version is whatever release of Fedora or RHEL it first appeared in. You would want to make a new subclass here, yes. I'd be happy to help with that if you need pointers.

This comment has been minimized.

@dustymabe

dustymabe Dec 15, 2017

Contributor

I can take a stab at it. would I have to create a new subclass for the other branches as well?

@dustymabe dustymabe force-pushed the dustymabe:master branch 2 times, most recently from 228f6f3 to df70037 Dec 15, 2017

if self.use_system_defaults:
return "# Firewall configuration\nfirewall --use-system-defaults\n"
else:
return F20_Firewall.__str__(self)

This comment has been minimized.

@clumens

clumens Dec 18, 2017

Collaborator

This is going to trip up whoever has to make a new class that inherits from this one and needs to do their own __str__ method, but I don't see that we can do much about that right now.

# use-system-defaults
self.assert_parse("firewall --use-system-defaults\n")

This comment has been minimized.

@clumens

clumens Dec 18, 2017

Collaborator

I'd like to see an additional test for when you use --use-system-defaults and some other option, to verify that part works. That could also help out with the other comment I left.

This comment has been minimized.

@dustymabe

dustymabe Dec 18, 2017

Contributor

passing something like firewall --use-system-defaults --enabled --service=ssh,smtp,ftp probably doesn't make sense, though. the --use-system-defaults is literally like a short circuit that says 'do nothing'. Do you agree? What are you looking for here?

This comment has been minimized.

@jkonecny12

jkonecny12 Dec 19, 2017

Collaborator

If it doesn't make sense than it should be an exception.

This comment has been minimized.

@clumens

clumens Dec 19, 2017

Collaborator

That's one possibility - yeah. An exception could be raised.

I know it doesn't make sense. What I'm looking for is a test to make sure that the thing that doesn't make sense is being handled correctly.

This comment has been minimized.

@dustymabe

dustymabe Dec 19, 2017

Contributor

ok, so you want me to raise an exception when someone provides --use-system-defaults and another option?

This comment has been minimized.

@dustymabe

dustymabe Dec 19, 2017

Contributor

I looked into this quite a bit today. I really tried to figure out how I could use argparse mutual exclusion to achieve this goal but unfortunately it seems the way things are wired up in anaconda (i.e. the overiding of the add_argument function) does not lend itself well to this. Do you know of any other examples where exclusivity is required in anaconda?

Within this very same module one can pass --disable --enable and anaconda will parse it fine.

This comment has been minimized.

@dustymabe

dustymabe Jan 2, 2018

Contributor

bump

This comment has been minimized.

@clumens

clumens Jan 2, 2018

Collaborator

I probably wouldn't want to handle that directly though argparse. Typically what you would do is add a parse function that checks the use_system_defaults attribute and something else after the argparse stuff is done, and raises an exception if both are set.

In this case, I don't really see any convenient way to do it. You would basically have to test every other attribute, which would be some really tedious code and would break every time we added something else. I would suggest we handle this via documentation - tell the user that if they use --use-system-defaults, that's the only option that will take effect regardless of what the rest of the command line looks like.

And then for a test case, what I am looking for is something like this:

self.assert_parse("firewall --enabled --service=ssh --use-system-defaults", "firewall --use-system-defaults\n")

In other words, that the new __str__ method gets fully tested out.

Things like --disable --enable should be handled such that whatever is given last is what takes effect, though we are probably not testing that.

firewall: add --use-system-defaults arg to command (#1526486)
Needed for [1] where we would like to include firewalld
and configure firewalld in Atomic Host (in the ostree) and
have Anaconda leave the delivered "defaults" in place.

[1] https://pagure.io/atomic-wg/issue/401

Resolves: rhbz#1526486

@dustymabe dustymabe force-pushed the dustymabe:master branch from df70037 to 86e6b2d Jan 2, 2018

@dustymabe

This comment has been minimized.

Contributor

dustymabe commented Jan 2, 2018

ok rebased and updated based on code review comments. I think I did everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment