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

Improve front-end build process #792

Merged
merged 1 commit into from
Jul 7, 2017
Merged

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented May 12, 2017

Instead of checking in binaries like ui/bindata.go and
template/internal/deftmpl/bindata.go into git, we generate them
whenever make build is run. If they already exist, we only regenerate
them if they are outdated.

For folks that only want to make back-end changes ui/app/script.js is
checked in, so they do not have to build the front-end.

make build will use Docker to build the front-end. If someone prefers
to install all the dev dependencies on their local machine instead, one
can add the NO_DOCKER=true flag.

Happy for any feedback, especially GNU Make specific.

I will rebase this PR once #789 is merged into Master.

@fabxc
Copy link
Contributor

fabxc commented May 12, 2017

So if we want to compile AM, even if we don't change anything about the UI, we have to have the docker daemon running and build the container and download its deps, then compile the UI, before we can proceed building the Go binary?

@mxinden
Copy link
Member Author

mxinden commented May 12, 2017

@fabxc Yes, the first time compiling Alertmanager one has to have the docker daemon running. From there on only if the underlying *.elm files change. This is all hidden behind make build, which runs all the steps (Build frontend, build backend).

@brian-brazil
Copy link
Contributor

I don't think it's at all reasonable to have Docker installed and running to compile our binaries.

@stuartnelson3
Copy link
Contributor

I agree with @brian-brazil, I don't think docker should be a hard requirement. But, I do think it's reasonable to expect that if you want to compile alertmanager, you compile all of alertmanager (which includes the ui).

I would propose:

  • Providing our build container on quay.io or dockerhub
  • Changing the build invocation in the elm make file to use ?= and encapsulate the whole build command, so ELM_MAKE ?= docker run --rm quay.io/prom/elm elm-make. Users can then either use elm-in-docker or elm on their machine

Thoughts?

@fabxc
Copy link
Contributor

fabxc commented May 15, 2017

I agree with @brian-brazil, I don't think docker should be a hard requirement. But, I do think it's reasonable to expect that if you want to compile alertmanager, you compile all of alertmanager (which includes the ui).

Which means it will be a hard requirement :)

I think it's a rather contentious point. What exactly are the drawbacks and complications of including the compiled data that make it worth removing this here? From what I understood the bindata.go timestamp update issue has been fixed.

@mxinden
Copy link
Member Author

mxinden commented May 15, 2017

@fabxc It just adds an extra layer of complexity during frontend development and in addition is a frequent reason for merge conflicts. For me it is worth the dependency, but that's of course very biased as I have to have it installed anyways.

Just thought it would be a nice improvement. It is not very important to me, if you guys prefer we stick with the existing process.

@mxinden
Copy link
Member Author

mxinden commented May 15, 2017

@fabxc @stuartnelson3 @brian-brazil Another suggestion: We check in the script.js file. This way anybody just compiling Alertmanager or only working on the back-end side of Alertmanager does not need to have any front-end related dependencies (NodeJS, ELM, ELM-Format, ... Docker) but only needs Golang. What do you think?

@brian-brazil
Copy link
Contributor

That's fine from a backend standpoint.

I think it's still unreasonable to require Docker for anyone making frontend changes.

@mxinden
Copy link
Member Author

mxinden commented May 15, 2017

@brian-brazil Ok, with script.js checked in we can kick out the Docker commands and run ELM right away or add a NO_DOCKER env variable which makes it optional.

@stuartnelson3
Copy link
Contributor

stuartnelson3 commented May 15, 2017

My point with using ?= in the makefile was that you can install elm instead of docker to do your changes. So leaving the command as I stated above defaulting to using docker, but then a user could do ELM_MAKE="$(which elm-make)" make assets, so then the user can decide if they want to use docker or install elm on their machine.

@w0rm
Copy link
Member

w0rm commented May 30, 2017

I am highly against the manual bindata step. Quite often I run into an issue, when it does not produce the same file as CI. Both steps - the Elm compilation and bindata compilation should be automated.

@w0rm
Copy link
Member

w0rm commented Jun 27, 2017

I think that make build should just do what @damomurf expected it to do in #873, it should build everything from the sources. Having to do three steps for the frontend changes is a bad developer experience.

@mxinden mxinden changed the title Remove checked in binaries Improve front-end build process Jun 27, 2017
@mxinden mxinden changed the base branch from ui-rewrite to master June 27, 2017 09:40
@mxinden
Copy link
Member Author

mxinden commented Jun 27, 2017

I have updated the PR accordingly.


Instead of checking in binaries like ui/bindata.go and
template/internal/deftmpl/bindata.go into git, we generate them
whenever make build is run. If they already exist, we only regenerate
them if they are outdated.

When one makes changes to the front-end and runs the single command
make-build on the root level Makefile, the front-end is rebuild
accordingly.

make build will use Docker to build the front-end. If someone prefers
to install all the dev dependencies on their local machine instead, one
can add the NO_DOCKER=true flag.

For folks that only want to make back-end changes ui/app/script.js is
checked in, so they do not have to build the front-end.


I think this is a good compromise for both front-end and back-end developers.

@brian-brazil @fabxc @brancz Can one of you try out make build on your local machine?
@w0rm Can you try out NO_DOCKER=true make build on your local machine?

@brian-brazil Let me know what you think!

@fabxc
Copy link
Contributor

fabxc commented Jun 27, 2017

Cool, that sounds like a good approach 👍

@mxinden
Copy link
Member Author

mxinden commented Jun 27, 2017

I will take care of the circle-ci builds once we have settled on a build process.

Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

That sounds good to me. Thanks!

to set the environment flag `NO_DOCKER` to `true` and have the following
dependencies installed:

- [Elm ](https://guide.elm-lang.org/install.html#install)
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the hint. Fixed.

@w0rm
Copy link
Member

w0rm commented Jun 27, 2017

Having to commit script.js instead of bindata.go keeps the same issues:

  1. We have to rebase if two PRs generate this file
  2. We have to ensure that this file is the same when generated locally and on CI

@mxinden
Copy link
Member Author

mxinden commented Jun 27, 2017

@w0rm Yes, it is not perfect from a front-end perspective, but it is a compromise for back-end developers, because otherwise we force back-end developers to either have docker or all the Elm dependencies installed to be able to build Alertmanager.

I am happy for alternative solutions! But I think forcing these dependencies on back-end developers is not an option (See conversations above).

@w0rm
Copy link
Member

w0rm commented Jun 27, 2017

NO_DOCKER=true make build worked for me locally.

@w0rm
Copy link
Member

w0rm commented Jun 27, 2017

@mxinden what about recompiling the script.js only when we do releases?

@mxinden
Copy link
Member Author

mxinden commented Jun 28, 2017

@w0rm I don't understand what that would solve. Sorry. Whenever the back-end depends on changes in the front-end those changes would only be visible on every release, right. Not keeping the front-end in sync with the back-end sounds dangerous to me.

@w0rm
Copy link
Member

w0rm commented Jun 28, 2017

@mxinden it would solve two issues that I mentioned before. We can recompile script.js when backend depends on the frontend or when we release, but not for each subtle change on the frontend that is merged into the master branch.

@mxinden mxinden force-pushed the elm-docker branch 13 times, most recently from f1c5c06 to ba47527 Compare July 6, 2017 11:41
We generate binaries whenever `make build-all` is run. If they already
exist, we only regenerate them if they are outdated.

When one makes changes to the front-end and runs the single command
`make build-all` on the root level Makefile, the front-end is rebuild
accordingly.

`make build-all` will use Docker to build the front-end. If someone prefers
to install all the dev dependencies on their local machine instead, one
can add the `NO_DOCKER=true` flag.

For folks that only want to make back-end changes `ui/bindata.go` is
checked in, so they do not have to build the front-end. They still use
the `make build` command as before.
@mxinden
Copy link
Member Author

mxinden commented Jul 6, 2017

Gnu make uses file timestamps in order to determine whether a target should be rebuild or not depending on its prerequisites. This is great in day to day work, but not if you freshly check out the project. Then every file has the same timestamp and everything has to be rebuild.

This forced me to make the following changes:

  • Introduce the make build-all target, which will rebuild assets like the front-end code. make build stays untouched.
  • Check ui/bindata.go in, instead of script.js

I am sorry for all the back and forth around such a small problem. I would like to get this merged and solve any further improvements in a new PR.

@fabxc
Copy link
Contributor

fabxc commented Jul 7, 2017

👍

@mxinden mxinden merged commit 1001f49 into prometheus:master Jul 7, 2017
@mxinden
Copy link
Member Author

mxinden commented Jul 7, 2017

@stuartnelson3 @w0rm Whenever you build something for the front-end you can now run make build-all to build everything with all its dependencies in docker. Hope this helps! :)

hh pushed a commit to ii/alertmanager that referenced this pull request Jan 22, 2018
* Fix smartmon.sh info label consistency.

* Fix parsing of SMART-ID attributes <= 99.
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.

5 participants