Updates/refactors in the Filesystem v1beta2 API #143
Conversation
k8s-ci-robot
added
release-note
mauriciopoppe
force-pushed
the
fs-api-changes
branch
4 times, most recently
from
691a7d3 to
1a07b2e
Compare
|
These are the results of the FS integration tests with 1a07b2e: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ddebroy, mauriciopoppe 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 |
k8s-ci-robot
added
the
approved
|
@jingxu97 can you provide the LGTM? Looks great to me! Please squash before the merge. |
| // LinkPath creates a local directory symbolic link between a source path | ||
| // and target path in the host's filesystem | ||
| rpc LinkPath(LinkPathRequest) returns (LinkPathResponse) {} | ||
| // CreateSymlink creates a local directory symbolic link from a target path |
There was a problem hiding this comment.
creates a symbolic link, target_path pointing to source_path in the host filesystem.
Also I am thinking to name target_path to link_path to avoid confusion. What do you think?
There was a problem hiding this comment.
target is really confusing in many places. https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinka
There was a problem hiding this comment.
I think that target_path and source_path are ok, from the documentation it's pretty clear that target_path is pointing to source_path, I interpret the arrow like this (from target path to source path) because that's what ls says:
/tmp❯ touch source_path
/tmp❯ ln -s source_path target_path
/tmp❯ ls -la source_path target_path
-rw-r--r-- 1 mauriciopoppe wheel 0 Jun 4 12:41 source_path
lrwxr-xr-x 1 mauriciopoppe wheel 11 Jun 4 12:41 target_path -> source_path
There was a problem hiding this comment.
ok, then update comment a little bit with
create a symbolic link called target_path that points to source_path in the host filesystem (target_path is the name of the symbolic link created, source_path is the existing path).
| IsMountPoint: false, | ||
| Error: err.Error(), | ||
| }, err | ||
| klog.Errorf("Failed to forward to IsSymlink: %v", err) |
There was a problem hiding this comment.
What's the feedback here?
There was a problem hiding this comment.
oh, just small case in failed
There was a problem hiding this comment.
I think I set it as uppercase in many places, typically when we add logs statements they start with an uppercase letter right?
There was a problem hiding this comment.
I think the current format is lowercase. but definitely it is not consistent.
if there are two many of them, we can use different PR for it.
| // - it is a soft link and | ||
| // - the target path of the link exists. | ||
| func (APIImplementor) IsMountPoint(tgt string) (bool, error) { | ||
| func (filesystemAPI) IsSymlink(tgt string) (bool, error) { |
There was a problem hiding this comment.
need more comments on what is returned
if tgt path does not exist, it return error
if tgt path exists, but the source path tgt points to does not exist, it return false without error
There was a problem hiding this comment.
sorry, maybe add similar comments into API definition too?
mauriciopoppe
force-pushed
the
fs-api-changes
branch
from
1a07b2e to
484c217
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
4 participants
What type of PR is this?
/kind cleanup
/kind feature
What this PR does / why we need it:
Adds all the changes as per #133
Which issue(s) this PR fixes:
Fixes #133
Special notes for your reviewer:
This is on top of the disk API and the volume API PRs, we should merge those first.
Does this PR introduce a user-facing change?:
/cc @jingxu97 @ddebroy