Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore: First stage of access logging middleware. #1571

Merged
merged 1 commit into from
May 11, 2017

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 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 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.

3 participants