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

memoize self.state #241

Merged
merged 3 commits into from
Nov 1, 2019
Merged

memoize self.state #241

merged 3 commits into from
Nov 1, 2019

Conversation

igalic
Copy link
Contributor

@igalic igalic commented Oct 31, 2019

Pull Request (PR) description

we memoize the output of check_running_state.

this also helps clarify the name of the function
self.check_running_state, which now no longer modifies a class-global
variable, and instead just returns the state

This pull Request is a Regression and is caused by #232 / #225 (which the author naïvely approved)

This Pull Request (PR) fixes the following issues

Fixes #240

@alexjfisher
Copy link
Member

There are other places that call check_running_state but also try to return a cached result. I think these should be tidied up in this PR too?

eg
https://github.com/voxpupuli/puppet-firewalld/pull/241/files#diff-2f1bff63c57b42c24ff9e175199e576fR97

@igalic
Copy link
Contributor Author

igalic commented Oct 31, 2019

indeed, these checks are now superfluous.

we memoize the output of check_running_state.

this also helps clarify the name of the function
self.check_running_state, which now no longer modifies a class-global
variable, and instead just returns the state

This pull Request is a Regression and is caused by #232 / #225 (which the author naïvely approved)

Fixes #240
state is now memoized, so we don't need to call this function again
Especially after we removed the side-effect from it!
@alexjfisher
Copy link
Member

what did 813141c do?

@trevor-vaughan
Copy link
Collaborator

No functional or idempotency issues noted during acceptance testing via simp/iptables.

Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

I think this is correct now. Thanks @igalic!

@alexjfisher alexjfisher merged commit b6fabc7 into voxpupuli:master Nov 1, 2019
@igalic igalic deleted the fix/long-runtime branch November 4, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reoccurring firewall-cmd command execution
3 participants