search

ForgeRock / openam-community-edition

Access Management - AuthN, AuthZ, SSO, Fedaration
https://forgerock.github.io/openam-community-edition/
123 stars 60 forks source link

Start of porting bug fixes #37

Closed FireBurn closed 6 years ago

FireBurn commented 6 years ago

This is my first attempt

These patches have been compile tested, and I've set up an OpenAM instance using my scripts

They haven't been tested further than that I'd be grateful if you could take a look

If you're happy with them I'll work on some more of them tomorrow

FireBurn commented 6 years ago

I'm not sure my take on OPENAM-8643 fixes the issue

FireBurn commented 6 years ago

OPENAM-7583 has been the most awkward one so far, finally got it compiling and running a simple instance

aldaris commented 6 years ago

OPENAM-7958 looks good too

aldaris commented 6 years ago

OPENAM-8106 looks good.

aldaris commented 6 years ago

OPENAM-6562 and OPENAM-4743 looks good.

aldaris commented 6 years ago

OPENAM-8321 and OPENAM-8248 look good too.

aldaris commented 6 years ago

OPENAM-8951 and OPENAM-9216 are good too.

FireBurn commented 6 years ago

I'll see about fixing OPENAM-6373 tomorrow and also porting OPENAM-8117 too

Any idea about OPENAM-7125, OPENAM-5659 & OPENAM-9479 - they don't have any patches in the master branch and I can't view them in Jira either

aldaris commented 6 years ago

OPENAM-8258 looks good

FireBurn commented 6 years ago

I think I've fixed up OPENAM-6373, added in the line and the extra bits required to make it work

I don't think there was anything wrong with OPENAM-7063 and I've slotted in OPENAM-8117 after it

Lastly I removed the extra import from OPENAM-7938

FireBurn commented 6 years ago

I think I fixed up OPENAM-8643, it's compiling now at least

I've also tested changing the password for the amAdmin user, but this was only on a one node cell

aldaris commented 6 years ago

This review is getting a bit unmanageable, can we divide this up to smaller PRs?

FireBurn commented 6 years ago

To implement OPENAM-7362 I had to convert getAllServerIDs to be a Collection

Everything compiled OK, but I'm not sure if this will affect where it's used else where

FireBurn commented 6 years ago

If I close this PR, create a new one with the ones you've already OK'd then create seperate pull requests for each OPENAM fix?

aldaris commented 6 years ago

That's one way to go about it, but I fear some of these fixes may have dependencies inbetween. I'm happy to review 3-4 commits at a time in quick succession if that helps.

FireBurn commented 6 years ago

Let me know, how you'd like things split up, and I'll get it sorted

I'm on IRC too if you'd like to talk about anything

FireBurn commented 6 years ago

Closing this and starting again