Unicon / cas-adfs-integration

Two different methods of integrating CAS Server and Microsoft ADFS
Apache License 2.0
19 stars 7 forks source link

CAS4 upgrade #8

Closed mmoayyed closed 9 years ago

mmoayyed commented 9 years ago

Still need to update the sample and test, but thought to open this up for review.

mmoayyed commented 9 years ago

bump

dima767 commented 9 years ago

I almost pushed that Merge button ;-)

mmoayyed commented 9 years ago

We'll wait a few more days to allow everyone to review. At some point, we might merge on lazy consensus.

jtgasper3 commented 9 years ago

Have we got a client that actively wants it?

jtgasper3 commented 9 years ago

I'm confused by the versioning. Why is this a 1.0.0-SNAPSHOT when 1.0.0 is already a release?

jtgasper3 commented 9 years ago

I'm confused by the versioning. Why is this a 1.0.0-SNAPSHOT when 1.0.0 is already a release?

Looks like the cas-server-support-wsfederation/pom.xml got bumped, but not the parent one. I'd suspect this should get change to 1.1 or 1.0.1 before merging.

mmoayyed commented 9 years ago

The versioning structure of the project is confusing.

You have:

a parent pom at 1.0.0-SNAPSHOT

the parent pom has 2 modules: the wsfed module is at 1.0.1-SNAPSHOT the sample site is at ${cas.version}

So I dont know.

Typically, all modules are children of a parent pom, who all share the same version number. If you manually update and manage pom versions and untangle a child from its own parent, then it gets confusing to update and manage releases. All modules should point to the same parent. If modules dont inherit from the same parent, then they should not belong to the project.

Suggestions?

mmoayyed commented 9 years ago

To elaborate furthermore:

if you want a sample app that basically shows how the module can integrated with ADFS support, GREAT! It needs its own project.

If you want a sample overlay that is released alongside the same module structure which suggests to folks that they too can build overlays on top of it, then the sample becomes a child and is released alongside the same set.

Make sense, or am I off?

jtgasper3 commented 9 years ago

I never intended for there to be a parent pom... someone else added that. :)

With that said... They should be consistent...

jtgasper3 commented 9 years ago

if you want a sample app that basically shows how the module can integrated with ADFS support, GREAT! It needs its own project.

Define "own project"... Do you mean its own repo? Cause by definition a pom.xml is a project and that's what it has.

mmoayyed commented 9 years ago

Cool. Lets get to fixing :) I'll update this.

As for an active enthusiasts who wants it, yes, I have to dig up their name. At the apereo conference I was asked if the module supports CAS4, and think do have an email from one gentleman who did ask.

mmoayyed commented 9 years ago

By "own project" I mean its own repo, yes. If you want it to have an independent lifecycle from the rest of the application, then you dont really want it to be a children of the parent pom. It's a like stranger living in your house. He doesn't eat the same food, he doesn't use the same resources, he doesn't do laundry, guests come over and ask" who is this guy?" and we have to launch into a complicated explanation. He's just there waiting for a new apartment to be found, but it never is found :D

jtgasper3 commented 9 years ago

Oh man... :D

As we said in private chat a few days ago. I'm fine with it being a sub-project, but would like a comment saying "Implementers: comment out/remove this parent block to run this in a standalone mode...".

Does that still work? I'm guessing the enthusiast will appreciate it. ;)

mmoayyed commented 9 years ago

:)

So you mean, if I clone it the module and decide to run the sample app? That depends.

As a typical deployer, if I clone the module, I'd simply run mvn package at the root of the app, grab the war and deploy that. No further changes should be required, I don't think. That's the shortest path to success. Removing the parent doesn't get me much, other than the fact it allows me to possibly run the sample site against a different version of the ws-fed module, (which means not only I should remove the parent, I also have to change the versions as well because those are no longer inherited right?) and I dont know why I would want to do that. When I clone snapshot code, I always accept the risk of "it may not work". So I would want it to run against the code I downloaded in same transaction.

That's a long way of saying, sure! A comment can be added, but offhand, I don't understand why someone would want to do something like that?

mmoayyed commented 9 years ago

P.S: Apologies if some of this stuff is redundant or repetitive. This is evidence that I am losing brain cells :)

jtgasper3 commented 9 years ago

Typical use of the example module was "hey go grab the source (as a zip since most didn't get git), grab the example directory and that is your cas overlay source. Now. update the cas.properties and wsfed.xml, and figure out your services registry, then live long and prosper."

A newb could be running CAS and integrated with ADFS in less 30 minutes if they stuck with the wild card service entry.

mmoayyed commented 9 years ago

Makes good sense, thanks. I updated the module to include additional comments on how the sample site can be detached from the parent and run independently.

mmoayyed commented 9 years ago

Defaults are also removed for those that didnt make sense.

mmoayyed commented 9 years ago

I think you may have clicked into the commit. On the latest changes in that branch, I cant find any iml files.

jtgasper3 commented 9 years ago

Couple of items of polish, then I think it is good to go.

mmoayyed commented 9 years ago

Great, thank you @jtgasper3 for your review. Please merge when you find the chance. Pull is updated.

jtgasper3 commented 9 years ago

Thank you for taking this on.