Skip to content

Minor patch to tools/check_format.py to fix #2391 .#2454

Merged
mattklein123 merged 1 commit intoenvoyproxy:masterfrom
srikailash:master
Jan 26, 2018
Merged

Minor patch to tools/check_format.py to fix #2391 .#2454
mattklein123 merged 1 commit intoenvoyproxy:masterfrom
srikailash:master

Conversation

@srikailash
Copy link
Copy Markdown
Contributor

@srikailash srikailash commented Jan 25, 2018

Description:
Fixes #2391 .
Reason for Issue :
If a directory is passed to the script , current directory is changed and relative path is used .
When /tools/header.py is called with relative path it couldn't find the file .

Fix : fixed by using absolute path instead of changing directory and using relative path .

Risk Level:
Small bug fix for tooling

Testing:
Tested with this one-liner(./tools/check_format.py check source/common/stats) and seems to work fine

Docs Changes:
N/A

Release Notes:
Now directories can also be passed as an argument to /tools/check_format.py .

[Optional Fixes #Issue]

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! does this fix #2391? If so can you mark the description as such? Also please fix DCO.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please remove commented out code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hello Matt , thanks for looking at it . pushed a fix with removing commented code . Yes , it fixes #2391 , will add that to description . can you please elaborate on what fixing DCO means ?

@srikailash srikailash changed the title /tools/check_format.py fails to check for directories. (https://github.com/envoyproxy/envoy/issues/2391) Minor patch to tools/check_format.py to fix #2391 . Jan 26, 2018
…changing directory and using relative path .

Signed-off-by: sri kailash <sri.gandebathula@booking.com>
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.

@danielhochman @htuch any concerns here? Seems fine to me. Not sure why chdir was done in the first place (probably by me once upon a time).

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM; the only reason I can think of for the chdir is if we're using this from a consuming repository like you do at Lyft. If it works for you, ship it.

@mattklein123 mattklein123 merged commit 4f6d3b2 into envoyproxy:master Jan 26, 2018
mattklein123 added a commit that referenced this pull request Jan 26, 2018
…g directory and using relative path (#2454)"

This reverts commit 4f6d3b2.
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* move node_info_cache to common

* add a new method to also fetch peer metadata id.

* get peer node info inside root context

* add capability to disabel node cache

* make node pointer return const

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

'./tools/check_format.py check path/to/dir' fails

3 participants