Development environment with hot reloading #221

dpordomingo · 2019-07-21T16:43:00Z

fix src-d/sourced-ce#26
blocked by src-d/sourced-ce#158

The current way to develop the UI is very painful:

make build takes ~10 mins every time you run it after changing some CSS line (because it ruins the cached docker context)
the official way is not clearly documented and is affected by the changes we made in superset because source{d} CE. It is needed extra wiring to run it.

This PR proposes a new way to run an environment for development (in MacOS and Linux), running (see docs in the CONTRIBUTING.md):

$ make dev-prepare
$ sourced init local

If it is done in that way, the env takes ~2mins to be ready, and every hot-reload takes ~5sec or less.

TL;DR

You'll find below the rationale behind the decision made.
Core changes in this PR
- 1dc63ed Configure development environment with hot reloading
- 597842d Polish README.md, moving info to CONTRIBUTING.md
Some other things that could be done in a separated PR if you prefer so:
- 95de302 Remove make dev-patch; I didn't find a use-case now that we have this, wdyt?
- f32cd19 Restore superset/contrib/docker/docker-init.sh (unused since we joined init scripts)

More info

When incubator-superset is ran in development mode, it's usually started with local directories bound to some internal paths. Content from volumes —in linux—, is owned by the user from the host machine.
Since the incubator-superset container is built to be run with superset user, content from volumes might be not writable because superset user is not the owner of those mounted dirs (as described by apache/superset#6887).

Alternatives:

Run incubator-superset as root:

since it's not the expected way to run superset, it can be unexpected,
data which is written in the volumes (as node_modules or __pycache__) will be written as root, so it won't be writable from the host machine (without being root)

Run incubator-superset container as root, but run incubator-superset app with the user owning the bound data; to do so:

assumption: all the bound data is owned by the same user.
Change the UID of superset, to the UID of the owner of the bound data.
Make sure the entrypoint is run by superset user, owning the original container data and the data bound to directories in the host machine.
All the data written by superset, will use the UID of the user of the host, so it will be writable also by that user in the host.

Docker loads (by default) an extra docker-compose.override.yml file (docs multiple-compose-files), with that in mind, we can extend our official docker-compose.yml file with our custom config to let the user run source{d} CE in development mode with hot reloading.

dpordomingo · 2019-07-21T16:51:11Z

I tested an initial version of this PR in MacOS successfully, but I need to try again the chmod thing.
I will try it and confirm it here when done.

dpordomingo · 2019-07-21T16:52:12Z

@ricardobaeta this version should be way easier to run than the one you tried some weeks ago.

smacker · 2019-07-25T11:38:33Z

watcher

+  exit 1
+fi
+
+make --no-print-directory apply-patch


you listen for delete event but doesn't delete anything

good catch...
I don't want to process the event to remove files on the other side...
Do you think that would it be safe calling git clean -fd $(SUPERSET_DIR) while the developer is working there?
(I'm not used to this git clean, so I'm not sure if some changes in $(SUPERSET_DIR) could be lost because of the git clean)

If it is not, I'd just exclude the delete event, and document:

Elements deleted in srcd won't be deleted from sourced; you can stop the watcher and call it again instead.

I don't feel comfortable if something can remove my work without my explicit action.
So I don't think git clean is a good solution here.

Keeping stuff can cause SUPER WEIRD behavior. For example, a user does import from 'path/*' somewhere.

Can we just call rm if we received the event?

Well.... import *... sigh

I should dig more on how the delete event is logged (and also the move event, if it's different...) considering the different platforms...

What if it could be done in a separate PR, and documented in the meantime?

issue opened to be done separately #225

Can we just call rm if we received the event?

This shouldn't be hard, and it seems sensible and the most straightforward thing to do IMHO, isn't it?

Each watcher returns the detected changes in a different way, so it should be parsed the output in both cases.
Since it is not critical, and it's documented, I think we can merge and improve later,

Okay! Thanks for the explanation!

superset/contrib/docker/docker-entrypoint.sh

smacker · 2019-07-25T11:41:01Z

srcd/contrib/docker/docker-compose.override.yml

+version: '3.2'
+services:
+  sourced-ui:
+    user: 0:0


what does it do?

It runs the container as root, instead of as superset (the user inside, not the host one)

The reasoning is detailed in the PR description, section: "More info" (it is also added a link to an issue in Superset repo)

YAML supports comments, let's add short description there. Something like set user as root inside the container to allow writing on host filesystem

well... that's not (fully) true, at least not directly. The whole sentence should be something like:

Set user as root inside the container to change superset internal UID to be the same as the host user. It will provide write access to the data from the volumes inside and outside of the container.

The problem is that the user: 0:0 is not enough to "allow writing on host filesystem".
The user: 0:0 is (only) needed to change internal superset UID, and then run the /entrypoint.sh as superset, which will do provide the write access.

But if you feel comfortable with that other doc as you proposed, let's do it.

docs added by d3dbe90

Makefile

dpordomingo · 2019-07-29T17:08:18Z

I tested make dev-prepare in OSX and it's now working thanks to the new commit d14273d

Avoid UID hack on OSX -> as described in PR desc, there's no problem with UID on OSX
Replace sed with awk -> because arguments in BSD sed and GNU sed are different
Adapt how stat command is formatted in BSD
Adapt how the colors are requested in BSD and GNU echo

This is now ready for final validation @src-d/applications

Makefile

se7entyse7en · 2019-07-30T11:26:50Z

superset/contrib/docker/docker-entrypoint.sh

@@ -27,8 +40,8 @@ elif [ "$SUPERSET_ENV" = "development" ]; then
    celery worker --app=superset.sql_lab:celery_app --pool=gevent -Ofair &
    # needed by superset runserver
    (cd superset/assets/ && npm ci)
-    (cd superset/assets/ && npm run dev) &
-    FLASK_ENV=development FLASK_APP=superset:app flask run -p 8088 --with-threads --reload --debugger --host=0.0.0.0
+    (cd superset/assets/ && npm run dev-server -- --host=0.0.0.0 --port=8088 --supersetPort=8081) &


Could you please add a comment or explain how the ports here are being used? In particular port 8081.

It's explained in their docs apache/incubator-superset/CONTRIBUTING.md#building and it might be too much to explain this wiring... but let's try; wdyt about 885dad2?

watcher

se7entyse7en · 2019-07-30T11:40:31Z

watcher

+  exit 1
+fi
+
+make --no-print-directory apply-patch


Can we just call rm if we received the event?

This shouldn't be hard, and it seems sensible and the most straightforward thing to do IMHO, isn't it?

CONTRIBUTING.md

dpordomingo · 2019-07-30T16:09:25Z

Thanks, @carlosms
your suggestions were addressed by 22e22a2

- Add docs (CONTRIBUTING.md) - Add command: 'make set-override' - Add 'docker-compose.override.yml' to override sourced 'docker-compose.yml' - Add 'watcher' command - Change 'docker-entrypoint.sh' to: - (in Linux) Accept 'root' user, and swipe to the expected 'superset' user. - Serve the UI from 'webpack-dev-server' instead of 'flask'. Signed-off-by: David Pordomingo <[email protected]>

Signed-off-by: David Pordomingo <[email protected]>

Since 'make patch-dev' is no longer needed, we can get rid of it, and also delete 'srcd/superset/superset_config_dev.py' Signed-off-by: David Pordomingo <[email protected]>

Signed-off-by: David Pordomingo <[email protected]>

dpordomingo added the enhancement New feature or request label Jul 21, 2019

dpordomingo requested review from ricardobaeta and a team July 21, 2019 16:43

dpordomingo self-assigned this Jul 21, 2019

dpordomingo mentioned this pull request Jul 21, 2019

Use a custom docker-file.override.yml src-d/sourced-ce#158

Merged

dpordomingo force-pushed the dev-hot-reloading branch from 8f27929 to f32cd19 Compare July 25, 2019 01:37

smacker reviewed Jul 25, 2019

View reviewed changes

superset/contrib/docker/docker-entrypoint.sh Show resolved Hide resolved

smacker reviewed Jul 25, 2019

View reviewed changes

Makefile Show resolved Hide resolved

dpordomingo mentioned this pull request Jul 26, 2019

Delete files on delete watcher event #225

Open

dpordomingo force-pushed the dev-hot-reloading branch 2 times, most recently from bc5db1c to 1eff7f6 Compare July 29, 2019 17:02

dpordomingo requested review from a team and smacker July 29, 2019 17:08

dpordomingo force-pushed the dev-hot-reloading branch from 1eff7f6 to d14273d Compare July 29, 2019 17:52

se7entyse7en suggested changes Jul 30, 2019

View reviewed changes

se7entyse7en approved these changes Jul 30, 2019

View reviewed changes

carlosms reviewed Jul 30, 2019

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

dpordomingo requested a review from carlosms July 30, 2019 16:09

carlosms approved these changes Jul 30, 2019

View reviewed changes

dpordomingo added 4 commits July 31, 2019 07:14

Polish README.md, moving info to CONTRIBUTING.md

26b55b1

Signed-off-by: David Pordomingo <[email protected]>

Polish Makefile removing unused target 'patch-dev'

d88f81f

Since 'make patch-dev' is no longer needed, we can get rid of it, and also delete 'srcd/superset/superset_config_dev.py' Signed-off-by: David Pordomingo <[email protected]>

Restore contrib/docker/docker-init.sh to its initial state

ab8d628

Signed-off-by: David Pordomingo <[email protected]>

dpordomingo force-pushed the dev-hot-reloading branch from 22e22a2 to ab8d628 Compare July 31, 2019 05:15

dpordomingo merged commit 9ff32db into src-d:master Jul 31, 2019

dpordomingo deleted the dev-hot-reloading branch July 31, 2019 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Development environment with hot reloading #221

Development environment with hot reloading #221

dpordomingo commented Jul 21, 2019 •

edited

Loading

dpordomingo commented Jul 21, 2019

dpordomingo commented Jul 21, 2019

smacker Jul 25, 2019

dpordomingo Jul 25, 2019

smacker Jul 25, 2019

dpordomingo Jul 25, 2019

dpordomingo Jul 26, 2019

se7entyse7en Jul 30, 2019

dpordomingo Jul 30, 2019

se7entyse7en Jul 30, 2019

smacker Jul 25, 2019

dpordomingo Jul 25, 2019

smacker Jul 25, 2019

dpordomingo Jul 25, 2019

dpordomingo Jul 25, 2019

dpordomingo commented Jul 29, 2019 •

edited

Loading

se7entyse7en Jul 30, 2019

dpordomingo Jul 30, 2019

se7entyse7en Jul 30, 2019

dpordomingo commented Jul 30, 2019

Development environment with hot reloading #221

Development environment with hot reloading #221

Conversation

dpordomingo commented Jul 21, 2019 • edited Loading

TL;DR

More info

dpordomingo commented Jul 21, 2019

dpordomingo commented Jul 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpordomingo commented Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpordomingo commented Jul 30, 2019

dpordomingo commented Jul 21, 2019 •

edited

Loading

dpordomingo commented Jul 29, 2019 •

edited

Loading