[keycloak-dev] Merging node_modules into Keycloak repo

Fri May 12 03:34:10 EDT 2017

On 11 May 2017 at 22:06, Bruno Oliveira <bruno at abstractj.org> wrote:

> Some answers inline.
>
> On 2017-05-11, Stian Thorgersen wrote:
> > Oh and also I like to just run stuff from the IDE. I very rarely build
> > Keycloak from mvn unless I'm mocking about with the distribution parts.
>
> To run stuff from your IDE, you just run npm install inside the project.
> You don't need Maven for it.
>

I'm not super keen on that either. I'm not a Node dev and most of the folks
on the team and our contributors aren't. So this has to be Java folks
friendly.

>
> >
> > On 11 May 2017 at 19:17, Stian Thorgersen <sthorger at redhat.com> wrote:
> >
> > > I have 3 concerns around not checking in node_modules. If these are
> > > addressed I'm happy:
> > >
> > > * Productization - I believe not checking in the modules will mean they
> > > have to be available in some internal NPM repository or something.
> Those
> > > module not already available we'll have to add ourselves. Stan you
> would
> > > have to figure this out as we no longer have a dedicated prod team
> (that's
> > > us now!)
>
> Yep, I know. One of the reasons why run diff at each upgrade of these
> dependencies will sound like hell.
>

What diff are you talking about? RHSSO is just pulling in KC tags. That's
it. However, it might be that we're "cheating" the productization process
by checking in the modules.

>
> > > * Build time - just the basic Angular2 example adds overhead
>
> I won't take the road of Java vs Node.js. But pretty much you have the
> same issue with Maven, when you don't have dependencies installed.
> One way or another you have to download it from the internet.
>

Nah you don't. Because Maven has a global Maven repo on your box
(~/.m2/repository dude). As long as you've ran the build since the deps was
last changed you can just use the offline mode with Maven.

>
> > > * Offline mode - the Angular2 example at the moment can't be built when
> > > not online. It will sit around for a loooong time until it eventually
> times
> > > out. NPM modules would have to be checked in somewhere globally (not
> just
> > > inside target dir or something like that). Maybe they can just be
> placed
> > > inside the .m2 dir or something?!
>
> Isn't the same when you are disconnected and you try to run Maven?
>

Nah, see above.

>
> Anyways, I don't think we gonna reach any consensus about it. So, anything
> you guys think is the best for the project, go ahead. It's software, we
> can always change, sometimes break :)
>

I'm actually more happy with not checking it in if we can workaround the
issues and make it a Java dev friendly process.

Can all the stuff needed be retrieved with Maven plugins or would folks
have to install Node, NPM, etc, etc.. to build what is mostly a "Java
project"?

>
> > >
> > > On 11 May 2017 at 15:10, Bruno Oliveira <bruno at abstractj.org> wrote:
> > >
> > >> 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#is
> > >> suecomment-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
> > >> _______________________________________________
> > >> keycloak-dev mailing list
> > >> keycloak-dev at lists.jboss.org
> > >> https://lists.jboss.org/mailman/listinfo/keycloak-dev
> > >>
> > >
> > >
>
> --
>
> abstractj
>