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

Older versions of ipset don't support comments #30389

Merged
merged 2 commits into from Jan 15, 2016

Conversation

Projects
None yet
5 participants
@justinta

justinta commented Jan 15, 2016

This should also fix a problem where the module will report incorrectly if there
isn't a comment.

I have a feeling there is a better way to do this, so if anyone has a critique or a better way of doing this, I'm all ears :)

Addresses this failing test: http://166.78.178.63:8080/job/branch_tests/job/2015.8/job/salt-2015_8-linode-debian8/lastFailedBuild/

justinta89 added some commits Jan 14, 2016

justinta89
Older versions of ipset don't support comments
This should also fix a problem where the module will report incorrectly if there
isn't a comment.
justinta89

jfindlay added a commit that referenced this pull request Jan 15, 2016

Merge pull request #30389 from jtand/ipset
Older versions of ipset don't support comments

@jfindlay jfindlay merged commit 3ac3804 into saltstack:2015.8 Jan 15, 2016

5 checks passed

default Merged build finished.
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #12780 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #12473 — SUCCESS
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #11368 — SUCCESS
Details
jenkins/salt-pr-rs-ubuntu14.04-n Salt PR - RS Ubuntu 14 #8855 — SUCCESS
Details

@justinta justinta referenced this pull request Jan 15, 2016

Merged

Added else statements #30391

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Jan 15, 2016

Contributor

Why did this get merged?! We just merged a commit in this module here: #30292

We need to double-check with @thegoodduke here and make sure we're all on the same page so we're not stomping on each other's changes.

Contributor

cachedout commented Jan 15, 2016

Why did this get merged?! We just merged a commit in this module here: #30292

We need to double-check with @thegoodduke here and make sure we're all on the same page so we're not stomping on each other's changes.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 15, 2016

Collaborator

@thegoodduke could you please review this one and the fixes in #30391 to make sure it's good? Some tests were failing which is why these changes were made, sorry we didn't check with you.

Collaborator

basepi commented Jan 15, 2016

@thegoodduke could you please review this one and the fixes in #30391 to make sure it's good? Some tests were failing which is why these changes were made, sorry we didn't check with you.

@justinta

This comment has been minimized.

Show comment
Hide comment
@justinta

justinta Jan 15, 2016

@cachedout @thegoodduke, my bad I should have referenced #30292 and pinged him. This was after his change to address the failing test from it. We didn't pick it up because it was buried in the other failing tests at the time.

justinta commented Jan 15, 2016

@cachedout @thegoodduke, my bad I should have referenced #30292 and pinged him. This was after his change to address the failing test from it. We didn't pick it up because it was buried in the other failing tests at the time.

@@ -439,22 +444,31 @@ def check(set=None, entry=None, family='ipv4'):
ipaddress.ip_address(end))
entries = []
for network in networks:
entries.append(network.with_prefixlen)

This comment has been minimized.

@thegoodduke

thegoodduke Jan 26, 2016

what for here? Can you explain it, please. @jtand

@thegoodduke

thegoodduke Jan 26, 2016

what for here? Can you explain it, please. @jtand

This comment has been minimized.

@justinta

justinta Jan 26, 2016

I took that line out here #30391, I didn't mean to put it in. It was just a remnant from testing.

@justinta

justinta Jan 26, 2016

I took that line out here #30391, I didn't mean to put it in. It was just a remnant from testing.

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