Closed Bug 1344004 Opened 7 years ago Closed 7 years ago

Add a config file which the the UI uses to get the url to the API.

Categories

(Conduit :: General, enhancement)

Product:
Conduit
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: imadueme, Assigned: imadueme)

References

(
URL
)

Details

Attachments

(4 files)

autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).
59 bytes, text/x-review-board-request
davidwalsh
: review+
smacleod
: review+
Details
autoland: add nginx config and port to see how ui is being served from compiled files instead of the dev server (bug 1344004).
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland-ui: Ignore node_modules in .dockerignore to let yarn image build normally (bug 1344004).
59 bytes, text/x-review-board-request
davidwalsh
: review+
smacleod
: review+
Details
autoland-ui: update yarn lock to include copy-webpack-plugin (bug 1344004).
59 bytes, text/x-review-board-request
davidwalsh
: review+
smacleod
: review+
Details 
      No description provided.
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review118512

::: autoland/ui/src/assets/public_config.js:12
(Diff revision 1)
> + *     "API_URL": "https://api.autoland.service.mozilla.org"
> + *   };
> + */
> +
> +const AUTOLANDUI_CONFIG = {
> +  "API_URL": null // Uses the local dev host when null.

Single quotes to stay consistent.

::: autoland/ui/src/constants.js:1
(Diff revision 1)
> +export const API_URL = (() => {

```
export const API_URL = AUTOLANDUI_CONFIG['API_URL'] || `http://${widow.location.hostname}:9999`;
```
Attachment #8843036 - Flags: review?(dwalsh) → review-
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review118512

> ```
> export const API_URL = AUTOLANDUI_CONFIG['API_URL'] || `http://${widow.location.hostname}:9999`;
> ```

Woops, don't know what I was thinking there :P
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review118512

> Woops, don't know what I was thinking there :P

Heh, happens when you're focused on adjusting code you have and not stepping back.  No worries!
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review118688

Need to fix something....
Attachment #8843036 - Flags: review-
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review118706

This looks good to me, but I think david has the call here, feel free to just flag him for the changes.

::: commit-message-0c5ce:12
(Diff revision 2)
> +This commit also adds some cleanup to the UI code since we don't need
> +the template.ejs file anymore because of the neutrino upgrade a while back.
> +This also moves our "static static" assets to a folder called "assets"
> +to make the distinction clear.

just a heads up for the future - whenever your commit message includes `"also adds <something> unrelated"`, it should probably be its own commit.
Attachment #8843036 - Flags: review?(smacleod)
Comment on attachment 8843382 [details]
autoland: add nginx config and port to see how ui is being served from compiled files instead of the dev server (bug 1344004).

https://reviewboard.mozilla.org/r/117142/#review119242

::: commit-message-e38ad:1
(Diff revision 5)
> +autoland: add nginx config and port to see how ui is being served from compiled files instead of the dev server (bug 1344004). r?smacleod

Port 8888 is the UI devserver which is useful for normal development (quickly changing stuff and so on), port 8889 serves the UI directly from the build files, that port is useful for making sure that the UI files are served correctly similar to how they will be served in production. 99% of the time you can just use port 8888. If you want to double check your work before you push, run `docker-compose run yarn build` then `docker-compose up` then go to ipaddress:8889 to see everything working. 

For this patch the way to check it is "does everything work exactly like before", if it isn't working you should see a blank page or API requests won't work.
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119254
Attachment #8843036 - Flags: review?(dwalsh) → review+
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119432

Breaks a test

```
FAIL  test/AutolandController_test.js
  ● Test suite failed to run

    ReferenceError: AUTOLANDUI_CONFIG is not defined

      at Object.<anonymous> (src/constants.js:1:197)
```
Attachment #8843036 - Flags: review+ → review-
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119626

Again, I'll let david make the decision on this, unflagging.

::: autoland/ui/src/assets/public.config:1
(Diff revision 4)
> +/*

Naming this .config seems wrong - if neutrino has no way to include js files without processing them that seems like a serious deficiency. For example, what if we'd like to vendor in a third party library, etc.
Attachment #8843036 - Flags: review?(smacleod)
Comment on attachment 8843382 [details]
autoland: add nginx config and port to see how ui is being served from compiled files instead of the dev server (bug 1344004).

https://reviewboard.mozilla.org/r/117142/#review119628

looks fine to me.
Attachment #8843382 - Flags: review?(smacleod) → review+
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119626

> Naming this .config seems wrong - if neutrino has no way to include js files without processing them that seems like a serious deficiency. For example, what if we'd like to vendor in a third party library, etc.

I agree this is ugly; I'll spend some time today to see if there's a better way to hack neutrino.
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119730

::: autoland/ui/assets/scripts/public_config.js:1
(Diff revision 5)
> +/*

Maybe `public-config.js` to be consistent?

::: autoland/ui/src/constants.js:5
(Diff revision 5)
> -const hostname = window.location.hostname;
> -
> -const UI_PRODUCTION_HOSTNAME = 'autoland.mozilla.org';
> -const API_PRODUCTION_ORIGIN = 'https://api.autoland.mozilla.org';
> -const API_DEVELOPMENT_ORIGIN = `http://${hostname}:9999`;
> +// Use a stub config in testing
> +const AUTOLANDUI_CONFIG = AUTOLANDUI_CONFIG || {
> +  'API_URL': null
> +};
> +export const API_URL = AUTOLANDUI_CONFIG['API_URL'] ||

`AUTOLANDUI_CONFIG.API_URL`
Attachment #8843036 - Flags: review?(dwalsh) → review-
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119764

I don't like this strategy.  My main issue is that it's too reliant on ops.  Asking the ops person to change a `.js` file's string, even automating it, is too much because ops people are hard to get a hold of.  I propose this:

1.  We have a `config.js` file that we use locally
2.  We have a `config-prod.js` file that will be used in prod
3.  We have `yarn build` for local and `yarn buildprod`, the later copying `config-prod.js` over to the build folder once build is complete, i.e.:

```js
"buildprod": "neutrino build && cp config-prod.js build/config.js", // or wherever it lives
```

Steven, what are your thoughts on this?  That way ops only *ever* has to do `buildprod` and not worry about anything else.
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119764

The values that go in this config file should not change, unless the actual server this UI points at changes URL. So I *guess* either work, but ops will almost *never* change this file, if they do it will be because of something they did.

The file will also be generated by a template + puppet script that ops runs, they won't be poking at the file themselves. So I don't think we need to worry about any of that.

Your suggested approach also seems *more* complex to me... thoughts?
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119764

What if *we* want to add more to the config file?  Another key for this or that?  Then we have to notify ops?  We'll know if they want to swap around addresses so we can make a quick commit; it's much easier for them to get a change from us than us from them.
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119764

I can't think of any other configuration the UI would need, but that you'd want to have to rebuild the entire image and redeploy to change. In that case why not just make it a constant somewhere else.

The purpose of this config was to hold values that would need to change if you were deploying the UI to staging, or dev, or a different instance out there. We need to be able to point it at different backends without rebuilding the entire container. Ops should be able to point it at our staging backend when it's deployed to staging.
Comment on attachment 8844579 [details]
autoland-ui: Ignore node_modules in .dockerignore to let yarn image build normally (bug 1344004).

https://reviewboard.mozilla.org/r/117968/#review119994

::: autoland/ui/.dockerignore:1
(Diff revision 2)
> +node_modules

Why do you have a node_modules folder anyway? Is this change really needed?

If you're not developing in docker, we should figure out a way to fix that.
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review119996
Attachment #8843036 - Flags: review?(smacleod)
Comment on attachment 8844579 [details]
autoland-ui: Ignore node_modules in .dockerignore to let yarn image build normally (bug 1344004).

https://reviewboard.mozilla.org/r/117968/#review119994

> Why do you have a node_modules folder anyway? Is this change really needed?
> 
> If you're not developing in docker, we should figure out a way to fix that.

All the commands and things are run through docker, in this case I have a node_modules folder so I can dive into the source of neutrino to figure out what it is doing. So the node_modules here isn't for executing code, but, rather for me to have a local copy of the source of our dependancies.
Comment on attachment 8844580 [details]
autoland-ui: update yarn lock to include copy-webpack-plugin (bug 1344004).

https://reviewboard.mozilla.org/r/117970/#review120058
Attachment #8844580 - Flags: review?(dwalsh) → review+
Comment on attachment 8844579 [details]
autoland-ui: Ignore node_modules in .dockerignore to let yarn image build normally (bug 1344004).

https://reviewboard.mozilla.org/r/117968/#review120060
Attachment #8844579 - Flags: review?(dwalsh) → review+
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review120064

::: autoland/docker-compose.yml:15
(Diff revision 7)
>        - "8888:80"
>        - "9999:9999"
>      volumes:
>        - ./docker/web/nginx-conf.d:/etc/nginx/conf.d:ro
>      depends_on:
> +      - yarn

Don't kill me but should we alphabetize this list?  *hides*

::: autoland/ui/src/constants.js:2
(Diff revision 7)
> -const hostname = window.location.hostname;
> -
> +// Use a stub config in testing
> +const AUTOLANDUI_CONFIG = AUTOLANDUI_CONFIG || {

We don't need to do the `||` stuff now that we're copying, right?  Also, instead of `const`, I think it would be helpful if we used `window.AUTOLANDUI_CONFIG`.
Attachment #8843036 - Flags: review?(dwalsh) → review-
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review120064

> We don't need to do the `||` stuff now that we're copying, right?  Also, instead of `const`, I think it would be helpful if we used `window.AUTOLANDUI_CONFIG`.

We still need to do the || for the tests to have a valid config to use. (Alternatively we could sprinkle window.AUTOLAND_ENV = {...}) in every test but I'd rather not).
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review120072
Attachment #8843036 - Flags: review?(dwalsh) → review+
Comment on attachment 8843036 [details]
autoland-ui: use a config file which can be managed by puppet to get the url to the api server (bug 1344004).

https://reviewboard.mozilla.org/r/116790/#review120088

rubber stamp for landing.
Attachment #8843036 - Flags: review+
Comment on attachment 8844579 [details]
autoland-ui: Ignore node_modules in .dockerignore to let yarn image build normally (bug 1344004).

https://reviewboard.mozilla.org/r/117968/#review120090

rubber stamp for landing.
Attachment #8844579 - Flags: review+
Comment on attachment 8844580 [details]
autoland-ui: update yarn lock to include copy-webpack-plugin (bug 1344004).

https://reviewboard.mozilla.org/r/117970/#review120092

rubber stamp for landing.
Attachment #8844580 - Flags: review+
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/automation/conduit/rev/65350fccda76
autoland-ui: use a config file which can be managed by puppet to get the url to the api server . r=davidwalsh,smacleod
https://hg.mozilla.org/automation/conduit/rev/0989ccf9b147
autoland-ui: Ignore node_modules in .dockerignore to let yarn image build normally . r=davidwalsh,smacleod
https://hg.mozilla.org/automation/conduit/rev/66b1b5324e06
autoland-ui: update yarn lock to include copy-webpack-plugin . r=davidwalsh,smacleod
https://hg.mozilla.org/automation/conduit/rev/6c7f559d5585
autoland: add nginx config and port to see how ui is being served from compiled files instead of the dev server . r=smacleod
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: