Skip to content

Conversation

@anmaxvl
Copy link
Contributor

@anmaxvl anmaxvl commented Jan 14, 2021

Do a better job at cleaning the logging driver path.
Add support for non-default hard drives.

@anmaxvl anmaxvl requested a review from a team as a code owner January 14, 2021 05:42
@anmaxvl anmaxvl force-pushed the user/maksiman/binary-io-better-path-handling branch from 86573c4 to 03e0c1f Compare January 14, 2021 05:42
}

// Strip any leading '\\' if the path contains a hard drive in it.
if strings.Contains(sanitized, ":\\") {

Choose a reason for hiding this comment

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

I'm confused about this section. Could :\\ be anywhere in the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like if we have multiple volumes, e.g. C:\path\to\something and D:\path\to\something. or am I missing something?

Comment on lines 133 to 124
if uri.Host != "" {
return "\\" + uri.Host + sanitized
}

Choose a reason for hiding this comment

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

Is it possible that uri.Host could already have a leading \\ like in the hard drive case above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

url.Parse will complain about back slashes in hostname, so things should fail before we even get here.

@anmaxvl anmaxvl force-pushed the user/maksiman/binary-io-better-path-handling branch from 03e0c1f to 54adb4d Compare January 26, 2021 07:26
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

Left some comments

@anmaxvl anmaxvl force-pushed the user/maksiman/binary-io-better-path-handling branch 2 times, most recently from 8e5e6b5 to f2e1d8c Compare January 28, 2021 04:57
@anmaxvl anmaxvl force-pushed the user/maksiman/binary-io-better-path-handling branch from f2e1d8c to 91416a5 Compare January 28, 2021 18:46
@anmaxvl anmaxvl force-pushed the user/maksiman/binary-io-better-path-handling branch from 91416a5 to bbae6da Compare January 30, 2021 00:27
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants