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

Refs #30365: Ignore symlinks for puppet executable #40

Merged
merged 1 commit into from Aug 7, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jul 10, 2020

No description provided.

@ekohl
Copy link
Member

ekohl commented Jul 10, 2020

Hmmm, tests are not happy

@ehelms
Copy link
Member Author

ehelms commented Jul 10, 2020

Took some finagling, @ekohl do check the test updates for the structural changes

kafo_parsers.gemspec Outdated Show resolved Hide resolved
@ehelms
Copy link
Member Author

ehelms commented Jul 30, 2020

@ekohl this needs a corresponding look

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

This removes the caching of @puppet_bin and I'd prefer to keep that. Any particular reason you're removing it?

@ehelms
Copy link
Member Author

ehelms commented Jul 31, 2020

IIRC when I was building the fix, this is cached at a class level which made it hard to test given in tests the paths needed to change to test different combinations. The call to find the bin file did not look expensive so I chose testing over performance gains from caching.

@ekohl
Copy link
Member

ekohl commented Jul 31, 2020

You can split the logic into two methods: find_puppet_bin and puppet_bin where the latter only performs the caching.

@ehelms
Copy link
Member Author

ehelms commented Aug 6, 2020

@ekohl Logic is now split

@ekohl ekohl merged commit f590048 into theforeman:master Aug 7, 2020
@ekohl
Copy link
Member

ekohl commented Aug 7, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants