Skip to content

Rename access log streams to make it more friendly to posix terminology#15721

Merged
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
davinci26:accessLogRename
Mar 29, 2021
Merged

Rename access log streams to make it more friendly to posix terminology#15721
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
davinci26:accessLogRename

Conversation

@davinci26
Copy link
Copy Markdown
Member

@davinci26 davinci26 commented Mar 26, 2021

Commit Message:
Fixes #15708

Risk Level: Low (rename only)
Testing: Just refactor

Sotiris Nanopoulos added 2 commits March 26, 2021 14:52
rename accesslog url types
2d46a0d 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Merge remote-tracking branch 'upstream/main' into accessLogRename
840ade3 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Mar 26, 2021

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15721 was opened by davinci26.

see: more, trace.

@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Mar 26, 2021

waiting to see what the CI thinks of the changes.

Sotiris Nanopoulos added 2 commits March 26, 2021 15:57
fix format
c56a6c0 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
more format
ee34d52 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@mattklein123 mattklein123 assigned phlax and mattklein123 and unassigned lizan Mar 26, 2021
@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Mar 27, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Mar 27, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15721 (comment) was created by @davinci26.

see: more, trace.

@davinci26 davinci26 marked this pull request as ready for review March 27, 2021 19:12
@davinci26 davinci26 requested a review from mattklein123 as a code owner March 27, 2021 19:12
mattklein123
mattklein123 requested changes Mar 28, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up. I agree this is a big improvement. Just a small comment. cc @phlax to see if there are any other comments. Thank you!

/wait

make case consistent
fe3bfb8 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
mattklein123
mattklein123 approved these changes Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM. @phlax any additional requests/comments?

phlax
phlax approved these changes Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@phlax phlax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm - with the caveat that im still wondering whether we can just use Stderr, Stdout and File rather than FileAccessLog etc

the less we make users type/remember the better

@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Mar 29, 2021

Re: the caveat of renaming.

I thought that we discussed it on the issue and we agreed that name consistency with other loggers is more valuable than shortening the name.

@phlax
Copy link
Copy Markdown
Member

phlax commented Mar 29, 2021

I thought that we discussed it on the issue and we agreed that name consistency with other loggers is more valuable than shortening the name.

the consistency i had in mind was more with FileAccessLog and then it occurred to me that this has just been added also

not sure about consistency with other protos tbh

not a blocker - just mho

@mattklein123 mattklein123 merged commit 98fb5d6 into envoyproxy:main Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phlax phlax phlax approved these changes

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@phlax phlax

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use Stdout and Stderr in canonical proto urls for logging

4 participants

@davinci26 @phlax @mattklein123 @lizan