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.

Sign up for GitHub

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

Jump to bottom

Replace evalSymlink() with powershell command for subPath_windows.go #92328

Merged
merged 1 commit into from Jul 13, 2020
Merged

Replace evalSymlink() with powershell command for subPath_windows.go #92328

merged 1 commit into from Jul 13, 2020

Conversation

jingxu97
Copy link
Contributor

@jingxu97 jingxu97 commented Jun 19, 2020 •
edited

In golang, evalSymlink() does not work if windows disk driver letter is
not assigned. Replace this function with a powershell command to work
around this issue.

This PR address the issue kubernetes/utils#172

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 19, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 19, 2020
@jingxu97
Copy link
Contributor Author

/retest

ddebroy
ddebroy reviewed Jun 22, 2020
View reviewed changes
pkg/volume/util/subpath/subpath_windows.go Outdated
return "", err
}
linkedPath = strings.TrimSpace(string(output))
if strings.HasPrefix(linkedPath, "Volume") || strings.HasPrefix(linkedPath, "\\??\\Volume") {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need the first condition: strings.HasPrefix(linkedPath, "Volume") ? You can potentially look for other Windows prefixes like \\.\Volume or maybe even \\ for generic UNC path handling.

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 output from powershell gives Volumexxxx as output without \

Copy link
Member

Choose a reason for hiding this comment

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

Ref about various path prefixes: https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code here by adding a volume prefix function. @ddebroy PTAL.

@jingxu97 jingxu97 force-pushed the June/evalpath branch 2 times, most recently from 75ec703 to 629d1a6 Compare June 23, 2020 20:01
@jingxu97
Replace evalSymlink() with powershell command for subPath_windows.go
b8ee3d2 
In golang, evalSymlink() does not work if windows disk driver letter is
not assigned. Replace this function with a powershell command to work
around this issue.
@jingxu97 jingxu97 mentioned this pull request Jun 23, 2020
Closed
@jingxu97
Copy link
Contributor Author

/retest

3 similar comments
@jingxu97
Copy link
Contributor Author

/retest

@jingxu97
Copy link
Contributor Author

jingxu97 commented Jul 2, 2020

/retest

@jingxu97
Copy link
Contributor Author

jingxu97 commented Jul 8, 2020

/retest

@ddebroy
Copy link
Member

ddebroy commented Jul 8, 2020

Thanks! /lgtm

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 9, 2020
@jingxu97 jingxu97 added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 9, 2020
@jingxu97 jingxu97 added the kind/bug Categorizes issue or PR as related to a bug. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jul 9, 2020
@jingxu97 jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2020
@spiffxp spiffxp added this to the v1.19 milestone Jul 9, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

13 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 1bcf42b into kubernetes:master Jul 13, 2020
@jingxu97 jingxu97 mentioned this pull request Jul 23, 2020
Merged
@jingxu97 jingxu97 mentioned this pull request Nov 20, 2020
Merged
k8s-ci-robot added a commit that referenced this pull request Dec 1, 2020
@k8s-ci-robot
Merge pull request #96729 from jingxu97/automated-cherry-pick-of-#92328-
cc92637 
#93707-upstream-release-1.18

Automated cherry pick of #92328: Replace evalSymlink() with powershell command for #93707: Fix issue in evaluating symlink path for subpath
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddebroy ddebroy ddebroy left review comments

@jsafrane jsafrane Awaiting requested review from jsafrane

@msau42 msau42 Awaiting requested review from msau42

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jingxu97 @k8s-ci-robot @ddebroy @fejta-bot @spiffxp