[keycloak-dev] Merging node_modules into Keycloak repo

[Bruno Oliveira]

On 2017-05-11, Stan Silvert wrote:
> On 5/10/2017 6:14 PM, Bruno Oliveira wrote:
> > On 2017-05-10, Stan Silvert wrote:
> > > The very thought of $subject seems like heresy.  Why check in something
> > > that is normally pulled using npm?
> > >
> > > We have Angular 2 examples in Keycloak now.  In the not-too-distant
> > > future, our Account Management console will be written in Angular 2.  So
> > > node_modules has to be there somehow.
> > >
> > > There are basically two options:
> > > 1)  Merge node_modules into the Keycloak repo.
> > > 2)  Don't merge and then run npm install at build time.
> > >
> > > Productization standards push toward option #1.  We need to have
> > > consistent, repeatable builds.
> > >
> > > But I'm looking for reasons that #1 might be bad.  I can't come up with
> > > a rational reason to do #2 except that it saves disk space.
> > My 2 cents here. I think #1 is bad for the following reasons:
> My instinct is that it's bad too.  But I need to play devil's advocate.
> >
> > 1. That's not the convention for Node.js development. Think about Java
> > devs committing JARs to our repo. Certainly that would be terrible.
> That's just the heresy argument.

Not really. It's also the way which QE thinks is the way to go (https://github.com/keycloak/keycloak-quickstarts/pull/25#issuecomment-300751341)

> > 2. Code review becomes a PITA at every dependency update.
> If you are talking about review in GitHub, that shouldn't be a problem.
> GitHub uses linguist to hide vendor files so that it doesn't mess up your
> review.  All you see is the file name.  By default, it doesn't show you the
> whole file.  Strangely enough, the linguist guys see checking in js
> libraries as a common practice:
> https://github.com/github/linguist#vendored-code

Well, you're assuming that we should all do our code reviews using only GH.

> > 3. Merge conflicts. Just think about multiple devs contribution to the
> > same repo and updating their modules.
> Maybe I'm missing something here.  Wouldn't that be really rare if it ever
> happened at all (in our case)?

Not if every developer decide to run npm update :)

> > 4. People could manually change these modules and you would never know
> > if the change is a consequence of `npm update` or a manual change.
> I really hope nobody would be dumb enough to do that!
>
> But come to think of it, this would be a security issue if someone did it on
> purpose.   Someone could sneak malicious code into our repo disguised as a
> legit PR.  It wouldn't be easy to catch during code review.

Trust me, if people wanted to do that. They wouldn't be dumb just
a single line

> > 5. This only makes sense on scenarios where dev cannot rely on npm
> > dependencies.
> >
> >
> > IMO option #2 would be the most viable. Projects from RedHat already do
> > this[1] with some success. So I don't see any need for it.
> >
> > [1] - https://github.com/aerogear/aerogear-unifiedpush-server/blob/02b133ffb49677effa347788c28c392ed3f275f0/admin-ui/pom.xml#L42
> Doesn't this make the build a LOT slower?

If slowness is the major concern, we should commit every single JAR
inside the repo and call it a day. But I don't think we do that, right?
Plus, Travis can cache the dependencies[1].

[1] - https://docs.travis-ci.com/user/caching/

>
> How is Aerogear dealing with productization?

You have to ask them, more precisely Matthias.

> >
> >
> > > Any thoughts?
> > > _______________________________________________
> > > keycloak-dev mailing list
> > > keycloak-dev at lists.jboss.org
> > > https://lists.jboss.org/mailman/listinfo/keycloak-dev
> > --
> >
> > abstractj
>

--

abstractj