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

Add --hydra-config flag to override concurrency from CLI #632

Merged
merged 3 commits into from
Apr 11, 2021

Conversation

patcon
Copy link
Contributor

@patcon patcon commented Mar 29, 2021

Related: #131

Thanks for considering, and thanks for all your work on maintaining this tool! :)

@patcon patcon requested a review from gjtorikian as a code owner March 29, 2021 20:15
patcon added a commit to hyphacoop/handbook that referenced this pull request Mar 29, 2021
@patcon
Copy link
Contributor Author

patcon commented Mar 29, 2021

Ah, I think I may have found a bug. Seems that when JSON config was being merged from --typhoeus-config, it was string keys being merged with symbolized keys internally. Not sure how gracefully this was handled. There were no errors when this happened to typhoeus, but hydra was throwing errors about "max_concurrency" not being valid option.

Screen Shot 2021-03-29 at 5 28 35 PM

(Note the double values at the end of the output debug string in screenshot)

If we instead parse JSON with JSON.parse(foo, { symbolize_names: true }), then these merge more cleanly. Going to do that, but lemme know if it should be in a separate PR)

@patcon
Copy link
Contributor Author

patcon commented Mar 29, 2021

Perhaps you could advise on whether this is an acceptable fix, before I start investigating/resolving these test failures. Seems to be an issue with checking a value called "myValue", which may be related to symbolization

@gjtorikian
Copy link
Owner

Ah, thanks for adding this!

I'm confused why the keys suddenly need to be symbolized, but I don't think that has any effect on the end user, so I'm fine with it. And yes, it looks like the test just needs to be turned into a symbol key rather than a string.

@patcon
Copy link
Contributor Author

patcon commented Mar 31, 2021

yay! thanks! will fix that 🙌

(Might be a JSON stdlib change way back, maybe was an old ruby version that worked the other way)

dcwalk added a commit to hyphacoop/handbook that referenced this pull request Apr 8, 2021
* Create holidays.md

* Update SUMMARY.md

* restructure to finance section

* fix link to finance

* Added pending concurrency config.

See: gjtorikian/html-proofer#632

* Moved to using html-proofer fork until upstream merge happens.

* Removed misplaced typhoeus config.

* Added slow version of html-proofer run. Documentation.

* Update holidays.md

* Update README.md

Co-authored-by: Patrick Connolly <[email protected]>
@patcon
Copy link
Contributor Author

patcon commented Apr 8, 2021

k pushed fix! Should be good to merge. Thanks @gjtorikian 🙌

@gjtorikian
Copy link
Owner

Thanks again!

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.

2 participants