Skip to content

tooling: Cleanups and utils#17477

Merged
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:tooling-cleanups-utils
Jul 26, 2021
Merged

tooling: Cleanups and utils#17477
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:tooling-cleanups-utils

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Jul 25, 2021

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: tooling: Cleanups and utils
Additional Description:

some minor cleanups and yaml utils

this also makes yaml a dependency of base.utils which will hopefully help to cut dep sprawl

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 25, 2021
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17477 was opened by phlax.

see: more, trace.

@phlax phlax marked this pull request as draft July 25, 2021 14:10
@phlax phlax force-pushed the tooling-cleanups-utils branch 4 times, most recently from 5c51988 to dbc2f8b Compare July 25, 2021 14:50
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jul 25, 2021

@phlax phlax force-pushed the tooling-cleanups-utils branch 2 times, most recently from 45422ee to 5c15509 Compare July 25, 2021 15:41
@phlax phlax changed the title [WIP] tooling: Cleanups and utils tooling: Cleanups and utils Jul 25, 2021
@phlax phlax marked this pull request as ready for review July 25, 2021 15:41
@phlax phlax force-pushed the tooling-cleanups-utils branch 4 times, most recently from e5b695f to 01483be Compare July 25, 2021 17:16
htuch
htuch previously approved these changes Jul 25, 2021
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, thanks!

@htuch htuch self-assigned this Jul 25, 2021
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jul 25, 2021

@htuch i pushed again not realizing you had reviewed - could you take another look - thanks!

(apologies for force-pushing after review)

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jul 26, 2021

fyi, the change was removing some extraneous code that i had added in the wrong place initially


def from_yaml(path: str) -> Union[dict, list, str, int]:
"""Returns the loaded python object from a yaml file given by `path`"""
with open(path) as f:
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.

Not that it matters a lot, but maybe we could move in the future to simpler pathlib read_text() here and write_text() below.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, agreed

theres some things i dont like about pathlib (like the duplication with os) but tbh i find myself thinking "hmm, i could do with some kind of object-oriented path representaiton" which is in fact exactly what pathlib is

if its ok - ill go through and do some pathlib cleanups en masse at some point

@phlax phlax merged commit 63933a3 into envoyproxy:main Jul 26, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Signed-off-by: Ryan Northey <ryan@synca.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants