Refactor configuration keys out of repository #8895
Conversation
…ow and cloud backend when building without required env vars
somebody1234
added
CI: No changelog needed
somebody1234
requested review from
PabloBuchu,
indiv0,
Frizi,
farmaazon,
MichaelMauderer,
vitvakatu and
kazcw
as code owners
|
@mwu-tow just a heads up that the relevant environment variables will need to be added to CI if this is merged in. the names of the environment variables are tentative though, so it probably won't be super urgent. |
| @@ -0,0 +1,34 @@ | |||
| /** @file An object containing globals to inject. */ | |||
There was a problem hiding this comment.
I wouldn't know from docs here that this is about injecting app configuration.
|
note: CI succeeding will not be sufficient to confirm that this works. we will need to download the built artifact and make sure it has the (correct) cloud environment visible (since it defaults to disabling cloud completely if the environment variables are not set.) |
| throw new Error(`${errorMessage} Response: ${text}.`) | ||
| } catch (error) { | ||
| throw new Error(`${errorMessage} Failed to read response: ${String(error)}.`) | ||
| if (REMOTE_LOG_URL != null) { |
There was a problem hiding this comment.
Personally I'm a fan of early return rather than if-pyramids but I suppose that's a stylistic choice.
There was a problem hiding this comment.
it's enforced by lints, but i agree. i kinda assumed we wanted to avoid early return because (i think) that's wojciech's preference, but we can definitely remove the lint enforcing if-pyramids - it's not like it's particularly useful. perhaps we can enforce that early return is only used at the very start though - because i guesss the point was that having returns in the middle of the function makes behavior surprising since you can easily miss it in a long function
There was a problem hiding this comment.
I think I do recall Wojciech prefers them, so I suppose it can stay 🤷
| throw new Error(`${errorMessage} Response: ${text}.`) | ||
| } catch (error) { | ||
| throw new Error(`${errorMessage} Failed to read response: ${String(error)}.`) |
There was a problem hiding this comment.
I realize this try/catch was here before the PR but this seems like it should be a try/catch/finally because wouldn't the first throw statement get caught by the catch and thus it'd say FOO Failed to read response: BAR Response: BAZ which is misleading since we did read the response?
|
@mwu-tow would you possibly be able to rename the |
|
should be ready to merge now but avoiding the merge until the env vars are updated on the GH side. |
@somebody1234 I've applied the renames: |
Pull Request Description
.gitignoreto allow*.envfiles inapp/ide-desktop/lib/dashboard/Important Notes
.env;.envwith prod backend;.pbuchu.env) on:npm run devinapp/ide-desktop/lib/dashboard/./run ide build./run ide2 build./run gui watchChecklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If GUI codebase was changed, the GUI was tested when built using./run ide build.