fix: introduce robust env configuration#165
Conversation
|
Visit the preview URL for this PR (updated for commit 710fe2a): https://mlab-sandbox--pr-165-02awbjp6.web.app (expires Thu, 05 Mar 2026 16:37:29 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 8e465d9b3e7c624aa653052fc53678329c9bfc07 |
5894630 to
513ebcb
Compare
|
Hi, sir, my name is Ashish. I did research and tried to get help from the organisation, but no feedback. From my understanding, many core measurement services like NDT7 are implemented in Go , I just want to confirm before moving further. |
|
Hi @Ashish-118! This is not quite the right place to gather information about us. Perhaps, you should take a look at https://github.com/m-lab/gsoc/ instead? Thanks! |
|
Thanks a lot sir. |
robertodauria
left a comment
There was a problem hiding this comment.
The approach looks good and much more robust. A few minor nits.
The #81 pull request introduced a nasty bug where we weren't using prod. The TL;DR of the bug is that `sed` was replacing both the initial assignment and the pattern, such that this code: ```javascript const placeholder = 'MLAB_PROJECT_PLACEHOLDER'; return placeholder === 'MLAB_PROJECT_PLACEHOLDER' ? 'mlab-staging' : placeholder; ``` became, at runtime: ```javascript const placeholder = 'mlab-oti'; return placeholder === 'mlab-oti' ? 'mlab-staging' : placeholder; ``` This was fixed in #164 but `sed` substitution is fragile. We need more robustness. I spent time thinking about this and this is what I concluded: 1. The biggest issue of the `sed` substitution is not the substitution per se, but a *lack of debuggability* by which the substitution only happens when deploying and cannot be tested locally. 2. Any solution that only applies when merging a pull request would be morally as bad as that one. It could fail without notice. 3. We now have `scripts/build.js` so we can take advantage of that. 4. Using `scripts/build.js` to select a committed `env.prod.js` _or_ `env.staging.js` is a *bad idea* because they might drift, thus putting us *back again* into a scenario where we only know something is wrong after merging a pull request. Based on this reasoning, with this diff I am proposing what I think is the most robust solution to the problem at hand: 1. We autogenerate `dist/js/env.js` from `scripts/build.js` 2. `scripts/build.js` *requires* a command line argument identifying the target deployment environment and this value is *parsed* and we reject invalid configurations (furthermore, it is always required to specify the argument so prod and staging cannot drift) 3. `dist/js/env.js` *only* contains a *single* variable named `mlabEnvName` and that can be *either* "prod" or "staging" (and it must always be like that, so drift is *not possible*) 4. The codebase *continues* to select the correct configuration inline depending on `mlabEnvName` values (which preserves the principle of locality and ensures the code that selects the config is *close to* the code it has effects upon) 5. Now the codebase *rejects* invalid or missing `mlabEnvName` and the "not configured case" leads to *hard failure* 6. Both deployment paths use the same pattern `npm run build -- $name` where `$name` is either "prod" or "staging" (this is arguably better than before where the staging deployment relied on a default) As a bonus, deploy `console.log` aggressively. This follows the *rule of transparency*. The operator must be able to easily and clearly see what URLs we're using. Easy inspection solves many problems and empowers to perform better manual testing. (Also, a user can now more easily report bugs.)
513ebcb to
710fe2a
Compare
The #81 pull request introduced a nasty bug where we weren't using prod.
The TL;DR of the bug is that
sedwas replacing both the initial assignment and the pattern, such that this code:became, at runtime:
This was fixed in #164 but
sedsubstitution is fragile. We need more robustness.I spent time thinking about this and this is what I concluded:
The biggest issue of the
sedsubstitution is not the substitution per se, but a lack of debuggability by which the substitution only happens when deploying and cannot be tested locally.Any solution that only applies when merging a pull request would be morally as bad as that one. It could fail without notice.
We now have
scripts/build.jsso we can take advantage of that.Using
scripts/build.jsto select a committedenv.prod.jsorenv.staging.jsis a bad idea because they might drift, thus putting us back again into a scenario where we only know something is wrong after merging a pull request.Based on this reasoning, with this diff I am proposing what I think is the most robust solution to the problem at hand:
We autogenerate
dist/js/env.jsfromscripts/build.jsscripts/build.jsrequires a command line argument identifying the target deployment environment and this value is parsed and we reject invalid configurations (furthermore, it is always required to specify the argument so prod and staging cannot drift)dist/js/env.jsonly contains a single variable namedmlabEnvNameand that can be either "prod" or "staging" (and it must always be like that, so drift is not possible)The codebase continues to select the correct configuration inline depending on
mlabEnvNamevalues (which preserves the principle of locality and ensures the code that selects the config is close to the code it has effects upon)Now the codebase rejects invalid or missing
mlabEnvNameand the "not configured case" leads to hard failureBoth deployment paths use the same pattern
npm run build -- $namewhere$nameis either "prod" or "staging" (this is arguably better than before where the staging deployment relied on a default)As a bonus, deploy
console.logaggressively. This follows the rule of transparency. The operator must be able to easily and clearly see what URLs we're using. Easy inspection solves many problems and empowers to perform better manual testing. (Also, a user can now more easily report bugs.)