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

Build Kubernetes UI from source. #8776

Closed
wants to merge 1 commit into from
Closed

Conversation

jackgr
Copy link
Contributor

@jackgr jackgr commented May 25, 2015

This PR changes the build scripts and adds a new build container based on npm, so that the web ui assets can be built from source, per this comment from @lavalamp on #7122 .

It does the following:

  • Cleans up the npm/gulp based UI build , which had a number of issues.
  • Creates a scriptable version of the UI build that doesn't run live reload and wait for changes.
  • Separates the build output from source arficats, which were mixed in the original.
  • Adds a nodejs container and scripts to run the UI build there, so that it's reproducible.

This PR is independent of the decision to check in generated UI build artifacts. It is not affected one way or the other by their presence or absence. All it does is clean up the UI build.

cc @bgrant0607, @preillyme, @bcbroussard.

@k8s-bot
Copy link

k8s-bot commented May 25, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

@bgrant0607
Copy link
Member

The main requirement is that someone should be able to git clone our repo, execute hack/build-go.sh, and launch a cluster via kube-up.sh. Any new dependencies should be pulled automatically, and no new commands should be necessary.

@bgrant0607
Copy link
Member

Assigned to @lavalamp to find someone else to review in detail.

@bgrant0607
Copy link
Member

823 files were changed. Could you please write a high-level description of what changes this PR makes?

@jackgr
Copy link
Contributor Author

jackgr commented May 26, 2015

After further discussion, it seems the best approach is to have the UI checked in by default, pre-built and ready to run, but to keep the rest of this work, so that the user can get a reproducible build of the UI, and to pick up all of the other bug fixes that went into it.

We will also move the check in of the built UI to a new PR that contains only build artifacts, so that this one is just the source changes, which are much smaller.

@lavalamp
Copy link
Member

Why does this PR remove the dependencies in third_party? We should keep all dependencies checked in. They should not be gotten at build time.

@yllierop
Copy link
Contributor

@lavalamp I just got done talking to @jackgr he is modifying the PR to not remove the dependancies and keep the defaults. Once it's re-pushed it should just be a small change touching only a small number of files. Give him a little bit longer to update this PR with the new approach and look again.

@yllierop
Copy link
Contributor

@bgrant0607 the 823 files changed number will be much lower after the next update to this PR that @jackgr is working on currently. Stay tuned.

@lavalamp
Copy link
Member

IMO having a stale UI comitted is worse than no ui. If we have built UI checked in, there needs to be a test that ensures it's not stale.

But I would prefer to have no (compiled) UI checked in-- in general we would like to eventually move to not having generated files checked in.

@lavalamp
Copy link
Member

There's no real need to keep it checked in, is there?

@jackgr
Copy link
Contributor Author

jackgr commented May 26, 2015

@lavalamp The concern, as I understand it, is that not having it checked in means that an extra step will be required to get it, that most developers will not bother with the extra step, and that therefore most developers will not see the UI.

@lavalamp
Copy link
Member

The concern, as I understand it, is that not having it checked in means that an extra step will be required to get it, that most developers will not bother with the extra step, and that therefore most developers will not see the UI.

Yeah, that's not important. No developer (ETA: of kubernetes itself-- I didn't mean no devs of anything) needs the UI, except those working on the UI. :)

@jackgr
Copy link
Contributor Author

jackgr commented May 26, 2015

@lavalamp @bgrant0607 PTAL. This PR now does nothing but make the UI build robust, per the top comment above, which is revised per request from @bgrant0607 to explain what it does.

The set of files changed is now quite small, and no new build tools are required for the user. If they want to build the UI, they'll need go-bindata, but that's the way it was originally. Nothing has changed in that regard.

The UI build is not part of the Travis or Shippable builds. It is part of the release build, since doing it there seemed appropriate. The other parts of the build are as they were originally. They don't care whether or not the UI is checked in, and they don't care whether or not the UI has been built.

We can make an independent decision regarding whether or not to keep the generated UI build output in the repo per #8830. I will submit a new PR that .gitignores the generated UI build output and removes it from the repo, and we can choose to merge it or not, depending on what we decide.

We can also make an independent decision regarding whether or not to have Travis and Shippable build the UI. I will submit a new PR that adds the UI build to the Travis and Shippable builds, and we can choose to merge it or not, depending on what we decide.

cc @preillyme

@yllierop
Copy link
Contributor

@lavalamp Let's discuss the merits of having the UI checked in by default on this issue: #8830

@yllierop
Copy link
Contributor

👍 LGTM

@lavalamp lavalamp removed their assignment May 27, 2015
@lavalamp
Copy link
Member

I think this change would be nice to get in, but I'm running low on cycles for this. Who wants to own our build scripts (now that Joe doesn't anymore)? That person should probably review this.

@ixdy? @quinton-hoole? I know you guys love shell ;) ;)

@jackgr jackgr changed the title WIP: Build Kubernetes UI from source. Build Kubernetes UI from source. May 27, 2015
@jackgr
Copy link
Contributor Author

jackgr commented May 27, 2015

Squashed commits and removed WIP from title.

@thockin
Copy link
Member

thockin commented May 27, 2015

This volume of new shell code so close to release - do we really have to do this?

@bgrant0607
Copy link
Member

To rephrase @thockin's question: What are the consequences of not doing this now? We're out of time.

@jackgr
Copy link
Contributor Author

jackgr commented May 29, 2015

We're going to break this PR up into smaller pieces that are easier to review and carry less risk.

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.

None yet

7 participants