Commons code reviews

  • More
  • CMN-209
  • summarized and closed
The new process for this project is to do review on Stash => https://stash.forgerock.org/projects/COMMONS/repos/forgerock-rest/pull-requests/48/overview

The new process for this project is to do review on Stash =>

https://stash.forgerock.org/projects/COMMONS/repos/forgerock-rest/pull-requests/48/overview

It should be on stash now, merge request #48

It should be on stash now, merge request #48

I agree with Phill - please create a pull request in Stash. You'll need to clone https://stash.forgerock.org/projects/COMMONS/repos/forgerock-rest/browse and then create a pull request between you...

I agree with Phill - please create a pull request in Stash.

You'll need to clone https://stash.forgerock.org/projects/COMMONS/repos/forgerock-rest/browse and then create a pull request between your private repo and the main repo. Chat with your local Git evangelist if you need help.

maybe, not aware of the engineering process for common project. I guess I should create a merge request?

maybe, not aware of the engineering process for common project. I guess I should create a merge request?

Doesn't this change have to be against Stash now?

Doesn't this change have to be against Stash now?

OPENAM-6167 HTTP headers should be case insensitive but Session REST endpoint is expecting lowercase
OPENAM-6167 HTTP headers should be case insensitive but Session REST endpoint is expecting lowercase
  • More
  • CMN-168
  • summarized and closed
New reviews on Stash: https://stash.forgerock.org/projects/COMMONS/repos/forgerock-http-framework/pull-requests/6/overview and https://stash.forgerock.org/projects/COMMONS/repos/forgerock-rest/pull...
  • More
  • CMN-206
  • summarized and closed
merged in https://stash.forgerock.org/projects/COMMONS/repos/forgerock-rest/pull-requests/24/overview https://stash.forgerock.org/projects/COMMONS/repos/forgerock-auth-filters/pull-requests/6/overview
But yes, I will describe http://sources.forgerock.org/static/mt0445/2static/images/wiki/icons/emoticons/smile.gif

But yes, I will describe

Users should definitely be surprised to get back "en", as they should get back "fr"!

Users should definitely be surprised to get back "en", as they should get back "fr"!

  • More
  • CMN-200
  • summarized and closed
  • More
  • CMN-208
  • summarized and closed
Merged to master via pull request: https://stash.forgerock.org/projects/COMMONS/repos/forgerock-audit/pull-requests/4
This approach was suggested as part of an earlier review as Matthew Swift wanted to avoid having a dependency on the servlet API. I'll leave this as it is for now but we can review this when moving...

This approach was suggested as part of an earlier review as Matthew Swift wanted to avoid having a dependency on the servlet API. I'll leave this as it is for now but we can review this when moving to CREST 3.

Yes, old habits die hard! I've created a pull request and immediately merged it as it's been reviewed here now: https://stash.forgerock.org/projects/COMMONS/repos/forgerock-audit/pull-requests/4

Yes, old habits die hard! I've created a pull request and immediately merged it as it's been reviewed here now: https://stash.forgerock.org/projects/COMMONS/repos/forgerock-audit/pull-requests/4

Ummmm.. Stash Pull Request?

Ummmm.. Stash Pull Request?

Can we instead use HttpContext httpContext = context.asContext(HttpContext.class); String ipAddress = httpContext.getRemoteAddress(); ? Context#toJsonValue() is going away in CREST3. (It probably ...

Can we instead use

HttpContext httpContext = context.asContext(HttpContext.class);
String ipAddress = httpContext.getRemoteAddress();

?
Context#toJsonValue() is going away in CREST3. (It probably should be deprecated in the 2.x branch.)

LGTM

LGTM

The default should be not to resolve. Is that the (new) default now for everybody?

The default should be not to resolve.

Is that the (new) default now for everybody?

CAUD-75: Allow /audit/access reverse DNS lookup to be turned off
CAUD-75: Allow /audit/access reverse DNS lookup to be turned off
Glad to be able to get rid of this!

Glad to be able to get rid of this!

CAUD-38 CMN-104 Flatten headers in CSVAuditEventHandler
  • More
  • CMN-205
  • summarized and closed
Committed to trunk, r3612. We can revert if we decide to leave json-fluent in forgerock-rest.

Committed to trunk, r3612.
We can revert if we decide to leave json-fluent in forgerock-rest.