[keycloak-dev] Merging node_modules into Keycloak repo

Fri May 12 04:48:27 EDT 2017

On 2017-05-12, Stian Thorgersen wrote:
> 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.

You just have to follow instructions like "run `npm install`". Is not
that hard ;)

>
>
> >
> > >
> > > 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.

Let's say that you checked in these dependencies, but I decided to run
"npm update". Based on what we have for the `package.json` at the
quickstarts, the most recent version of each dependency will be
download. And now, you have to track the change for each dependency.

Let's take for example this package.json(https://github.com/keycloak/keycloak-quickstarts/pull/25/files#diff-c51a8b39c47d3e583b72b31558d0aedbR12). You pretty much would need to know
what has changed for each dependency.

>
>
> >
> > > > * 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.

Yep, if you have nothing, and you are disconnected, there's no offline
dependency. What sucks and I believe is your point, is the fact
that Node don't have the concept of global repository for all
the dependencies.

>
>
> >
> > > > * 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"?

The idea behind running Maven is to automate the installation of Node dependencies
inside the project. But still, people prior to that people have to install Node.

>
>
> >
> > > >
> > > > 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
> >

--

abstractj