Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework UI build to improve caching and fix packaging issue #3353

Merged
merged 4 commits into from
Sep 26, 2018

Conversation

leth
Copy link
Contributor

@leth leth commented Sep 24, 2018

Stop baking the toolchain and dependencies into the build image. Instead, run the install step each time, but use volume mounts or CircleCI caching to keep the happy path fast.

Previously, some parts of the client (UI) directory were baked into the build image, and some parts were mounted or copied into the build environment. As a result, files baked into the build image require a two-step update for changes to take effect in CI.

Now, for dockerised builds, we pre-install very little into the build image and mount the whole directory into the build environment. However, we do overlay a volume on the node_modules folder to allow the standard build toolchain to be separate from the host build toolchain.

Non-dockerised builds (e.g. CI) are now more similar to the dockerised versions.

Fixes #3351

@leth leth requested review from fbarl and bboreham September 24, 2018 14:47
@leth leth force-pushed the rework-ui-build branch 3 times, most recently from 4681736 to 55709f4 Compare September 24, 2018 15:38
@bboreham
Copy link
Collaborator

Can you outline what you intended to do, so I don't have to reverse the intent out of the code?

@leth leth changed the title Rework UI build to improve caching Rework UI build to improve caching and fix packaging issue Sep 25, 2018
@leth
Copy link
Contributor Author

leth commented Sep 25, 2018

Sorry @bboreham! You're right, it was missing any explanation. Hopefully it makes sense now!

One improvement I'll make in a few minutes:

  • add a Makefile target for the build toolchain

@leth leth force-pushed the rework-ui-build branch 3 times, most recently from 7c5eb81 to 47d8164 Compare September 25, 2018 09:38
Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good from the bits I was able to understand :)

@bboreham
Copy link
Collaborator

bboreham commented Sep 25, 2018

In the CI build the cache is basically empty - do you think it will work better if we merge it, or still more work required?

https://circleci.com/gh/weaveworks/scope/9619

@leth
Copy link
Contributor Author

leth commented Sep 25, 2018

I think I need to update the tag for the UI build image in the CI config, to pull in the XDG_CACHE_HOME var, before things will work properly.

The cache dirs work in the local build; I dont think I've messed anything up, so it should work in CI too 🤞

Marcus Cobden added 3 commits September 26, 2018 10:41
Stop baking the toolchain and dependencies into the build image.
Instead, run the install step each time, but use volume mounts or
CircleCI caching to keep the happy path fast.

Previously, some parts of the client (UI) directory were baked into the build
image, and some parts were mounted or copied into the build environment.
As a result, files baked into the build image require a two-step
update for changes to take effect in CI.

Now, for dockerised builds, we pre-install very little into the build
image and mount the whole directory into the build environment.
However, we do overlay a volume on the node_modules folder to allow the
standard build toolchain to be separate from the host build toolchain.

Non-dockerised builds (e.g. CI) are now more similar to the dockerised
versions.
@fbarl
Copy link
Contributor

fbarl commented Sep 26, 2018

I think it would be great if we could merge the changes that fix the packaging issue, which is a blocker for having latest Scope in Weave Cloud (and https://github.com/weaveworks/service-ui/pull/3202 in particular) and then try to further improve the caching in a separate PR :)

Does that make sense @leth?

@leth
Copy link
Contributor Author

leth commented Sep 26, 2018

I'm just doing some tests to check that the caching stuff works; It should be ready to go after lunch!

@leth leth merged commit 1e67ef2 into master Sep 26, 2018
@leth leth deleted the rework-ui-build branch September 26, 2018 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants