-
Notifications
You must be signed in to change notification settings - Fork 115
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
Only call Oj.mimic_JSON when in a CLI run #779
Conversation
5ba4d92
to
4e089a2
Compare
@paracycle all good points. I've updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good for me from what I remember working in Krane long time ago, but I defer to @dturn since I may be missing some context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oj.mimic_JSON
brought a noticeable perf improvement (on the order of seconds) since this gem does a ton json processing. I'm ok with only including it in the CLI but please restore the lib/krane/oj.rb file.
I don't understand. I simply moved it's content inside |
Its a minor thing but, I think its better code organization, it also makes it a one liner to restore the behavior for people using Krane as a library. |
4e089a2
to
a83fbf1
Compare
Done. |
Context
We have a bunch of tests in our CI suite that require krane to test a few deploy stuffs. The problem is that requiring
krane
callsOj.mimic_JSON
which cause various issues to us.Change
I think krane should only patch JSON when it is ran as CLI, as it has better control of what's running. When used as a library, that's not a good idea to patch core classes like this.
NB: my knowledge of crane and Thor is very limited, so this PR might be totally incorrect.
cc @paracycle @Shopify/test-infra