Add unit tests around WEB_CONCURRENCY

[michaelherold]

This buildpack is inconsistent with the heroku/ruby buildpack's setting
of WEB_CONCURRENCY. It tries to guess a reasonable value for
WEB_CONCURRENCY, even if one is set manually in the application's
configuration.

Since this value is used, by default, by the Puma web server, this
behavior is undesirable. Newer Rails applications use both the
heroku/nodejs buildpack and the heroku/ruby buildpack because Rails
manages assets through Webpack.

This change respects any value that the customer sets for
WEB_CONCURRENCY via heroku config:set instead of blindly overwriting
the value. With this behavior, the customer can better control the
process-level concurrency for their applications.

I verified that this behavior does, indeed, occur by doing the
following:

1. heroku config:set WEB_CONCURRENCY=2 -a <myapp>
2. heroku run -a <myapp> bash
3. echo $WEB_CONCURRENCY

For step (3), the value was set to WEB_CONCURRENCY=1 instead of the
expected WEB_CONCURRENCY=2. I do not believe the buildpacks have an
order-dependence since the Ruby one does not override a set value.

[michaelherold]

It appears my understanding of what environment variables are set when you run a ps:exec was flawed. I see that none of the environment variables that you set in your application configuration are available. Thus, the unexpected behavior I was seeing wasn't correct.

I understand if this change isn't wanted, but it would be nice to see consistent behavior between different Heroku-created buildpacks. To me, the setting of the value unless it is already set leads to more understandability.

[dzuelke]

This is already the behavior:

heroku-buildpack-nodejs/profile/WEB_CONCURRENCY.sh

Line 8 in 664de44

concurrency=${WEB_CONCURRENCY-$(($available/$web_memory))}

sets the variable to the user-supplied value, then performs some sanity checks (too large or too small) on it.

Maybe keep the unit tests, they can't hurt, and remove the other two changes? ;)

[danielleadams]

@michaelherold Thanks for the PR! As @dzuelke said, the extra coverage would be great if you could fix the tests. Otherwise, do you mind if we close this?

[michaelherold]

This is already the behavior:

🤦 So it is. It was very easy to miss that since it's hidden in the function. And since it does some checking against the value, my sentinel value was being ignored.

the extra coverage would be great if you could fix the tests.

I removed everything but the tests, but Hatchet is still failing to work correctly on CI. I don't know why that's happening.

[danielleadams]

@michaelherold These are tests that we run internally to check integration with the Heroku platform. I recently moved the tests to Circle CI where I am not able to figure out how to run the test myself (they will continue to fail because you don't have access to the Heroku creds). If I can't figure this out, I'll open a pull request a run the tests on the next release. Thanks!

[danielleadams]

@michaelherold Can you rebase this? I can merge it it once it matches the base branch. Thanks!

[danielleadams]

@michaelherold can you rebase this?

[michaelherold]

Done!

…

On Tue, Jan 19, 2021 at 3:22 PM Danielle Adams ***@***.***> wrote: @michaelherold <https://github.com/michaelherold> can you rebase this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#876 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADHXAPFPAZ2I3OL4MKHNODS2XZY5ANCNFSM4UT67GUA> .