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

Only call Oj.mimic_JSON when in a CLI run #779

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Conversation

casperisfine
Copy link
Contributor

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 calls Oj.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

@casperisfine casperisfine requested a review from a team as a code owner January 8, 2021 11:55
lib/krane.rb Outdated Show resolved Hide resolved
lib/krane/cli/krane.rb Outdated Show resolved Hide resolved
@casperisfine
Copy link
Contributor Author

@paracycle all good points. I've updated the PR.

Copy link
Contributor

@douglas douglas left a 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.

Copy link
Contributor

@dturn dturn left a 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.

@casperisfine
Copy link
Contributor Author

but please restore the lib/krane/oj.rb file

I don't understand. I simply moved it's content inside krane/cli.rb. I'm fine restoring it and requiring it from krane/cli.rb but I don't really see the point.

@dturn
Copy link
Contributor

dturn commented Jan 8, 2021

but I don't really see the point.

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.

@casperisfine
Copy link
Contributor Author

but please restore the lib/krane/oj.rb file

Done.

@casperisfine casperisfine merged commit 0c79b76 into master Jan 11, 2021
@casperisfine casperisfine deleted the oj-mimic-json branch January 11, 2021 09:48
@timothysmith0609 timothysmith0609 temporarily deployed to rubygems January 20, 2021 20:07 Inactive
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.

6 participants