[keycloak-dev] Merging node_modules into Keycloak repo

[Stian Thorgersen]

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.

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!)
> * Build time - just the basic Angular2 example adds overhead
> * 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?!
>
> 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
>>
>
>