Skip to content

Conversation

@ldez
Copy link
Contributor

@ldez ldez commented May 9, 2017

Description

Restore access logging middleware.

Related to #1541, #1408, #1485

@ldez ldez added the WIP label May 9, 2017
@ldez ldez force-pushed the restore-access-logger branch from 5eed593 to 75f1fe3 Compare May 9, 2017 13:34
@ldez ldez added kind/enhancement a new or improved feature. and removed WIP labels May 9, 2017
@ldez ldez requested a review from timoreimann May 9, 2017 14:44
@ldez ldez added area/logs priority/P1 need to be fixed in next release labels May 9, 2017
@ldez ldez modified the milestone: 1.4 May 10, 2017
@ldez ldez removed the priority/P1 need to be fixed in next release label May 10, 2017
}

//-------------------------------------------------------------------------------------------------
// the next 3 function (SaveNegroniFrontend, NewSaveNegroniFrontend, ServeHTTP) are temporary,
Copy link
Contributor

@timoreimann timoreimann May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason we have the part from here to the rest of this file? It's not in the original PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the code evolve between the first merge and the revert restoration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I realized now that the functions were in the original PR but didn't include the Negroni part in the name.

Why the warning at the top and why change the names?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timoreimann we worked both on resolving this conflict. we had to do this to manage different types negroni and http handlers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @ldez
LGTM

@ldez ldez force-pushed the restore-access-logger branch from a1bae72 to d2c8824 Compare May 11, 2017 14:28
@ldez ldez merged commit 94f5b0d into master May 11, 2017
@ldez ldez deleted the restore-access-logger branch May 11, 2017 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/logs kind/enhancement a new or improved feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants