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

Update firewall manifests to use jump instead of action #372

Merged
merged 5 commits into from
Feb 14, 2024

Conversation

david22swan
Copy link
Member

As part of the Firewall module rewrite the functionality of the action attribute has been rolled into the jump attribute, the two of them both managing the Firewall jump value.

@joshcooper
Copy link
Contributor

Is the jump parameter supported by all versions of the firewall module listed in metadata.json?

@david22swan
Copy link
Member Author

david22swan commented Sep 14, 2023

jump was always present, action was created to represent 3 different possible values that can be passed to the iptables jump value but with the release of v7.0.0 action has been removed and jump is now one value.

It was an odd piece of code added to enforce the use of those 3 values above any of the others. There would likely be errors from this if used alongside versions of firewall less than 7, but is needed for versions greater than 7. You may need to label this backwards incompatible and do an x release

@joshcooper
Copy link
Contributor

There would likely be errors from this if used alongside versions of firewall less than 7, but is needed for versions greater than 7.

Should this be bumped to >= 7 and < 8?

"version_requirement": ">= 1.1.3 < 7.0.0"

@david22swan
Copy link
Member Author

There would likely be errors from this if used alongside versions of firewall less than 7, but is needed for versions greater than 7.

Should this be bumped to >= 7 and < 8?

"version_requirement": ">= 1.1.3 < 7.0.0"

Updated.
Sorry on the wait, was on PTO friday

@corporate-gadfly
Copy link
Contributor

Any thoughts on a release?

@david22swan
Copy link
Member Author

@joshcooper Could you take a look

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

Seems ok to me, but honestly I have no idea.

@austb
Copy link
Contributor

austb commented Sep 28, 2023

Since this will require an X release, I am planning to do a release of the changes that have landed on main before we merge this.

Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Looks good to go when we are ready for a major version bump

@corporate-gadfly
Copy link
Contributor

Looks good to go when we are ready for a major version bump

Any estimates on when approximately that might be?

@corporate-gadfly
Copy link
Contributor

Looks good to go when we are ready for a major version bump

Any updates? TY.

@rafiqueh
Copy link

rafiqueh commented Jan 4, 2024

Looks good to go when we are ready for a major version bump

Happy New Year. Any updates?

@david22swan
Copy link
Member Author

@austb Thoughts on when this could go in?

Copy link
Collaborator

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

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

@david22swan looks good to me. maybe correct BUDFIX? :D and I think we should keep this as breaking change because it bumps a dependency by a view versions.

As part of the Firewall module rewrite the functionality of the `action` attribute has been rolled into the `jump` attribute, the two of them both managing the Firewall jump value.
@david22swan david22swan changed the title (BUGFIX) Update firewall manifests to use jump instead of action (BACKWARDS-INCOMPATIBLE) Update firewall manifests to use jump instead of action Feb 14, 2024
@david22swan
Copy link
Member Author

Had BUGFIX just because I didn't have a ticket for it.
Does this look better?
Or at least more accurate

@bastelfreak
Copy link
Collaborator

Can you unpin the module in https://github.com/puppetlabs/puppetlabs-puppetdb/blob/main/.fixtures.yml#L22 ? Then tests should/could pass.

@bastelfreak bastelfreak merged commit bf22956 into puppetlabs:main Feb 14, 2024
20 checks passed
@h0tw1r3 h0tw1r3 changed the title (BACKWARDS-INCOMPATIBLE) Update firewall manifests to use jump instead of action Update firewall manifests to use jump instead of action Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants