-
Notifications
You must be signed in to change notification settings - Fork 304
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.
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
HPCC-31212 Add plane name to RemoteDirectory #18292
Conversation
https://track.hpccsystems.com/browse/HPCC-31212 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See general comments in dependent PR (#18275).
Let's revisit this when that review is complete/near complete.
1d9ed27
to
e3100fa
Compare
cd0ff8d
to
e9470c7
Compare
The windows build error seems not related to this PR. The second commit does not need to be reviewed because it is a temporary build fix for the issue in the latest master repo and will be removed before merging. |
This PR has been rebased to the latest master where the windows build error has been fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wangkx - given the route of other changes to MoveExternalFile/DeleteExternalFile/CreateExternalDirectory (i.e. adding a plane param), and despite RemoteDirectory not being a great name (and despite me suggesting a new method name be introduced), I think for consistency with other changes, this change should take the same route, i.e. a plane option should be added to RemoteDirectory too, and it and/or the hostname if provided should be validated against dropzone planes.
@jakesmith I am not sure I fully understand your requirement. Based on the comment, I will add a fsRemoteDirectory_v2, as I did for MoveExternalFile, etc. Should I keep the new fsListPlaneDirectory in this PR or it is no longer needed? |
1. Create new fsListPlaneDirectory which = fsRemoteDirectory + plane name. 2. Revise implementation to call the same code from both fsRemoteDirectory and fsListPlaneDirectory. Revise based on review: Replace the fsListPlaneDirectory with fsRemoteDirectory_v2 Signed-off-by: wangkx <[email protected]>
@jakesmith I made the changes from fsListPlaneDirectory to fsRemoteDirectory_v2. I also modified the PR title and description. Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wangkx - looks good.
@ghalliday please merge this PR. |
The STD.File.RemoteDirectory lists files in one landing zone folder. The existing method can only specify a landing zone using host name. In this PR, a landing zone can be specified by plane name.
Type of change:
Checklist:
Smoketest:
Testing: