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.

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

[WFCORE-6559] Allow Remove-Item command to delete directories with lo… #5721

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

yersan
Copy link
Collaborator

@yersan yersan commented Oct 12, 2023

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Oct 12, 2023
@bstansberry
Copy link
Contributor

@yersan What about UNC?

From https://learn.microsoft.com/en-ca/windows/win32/fileio/maximum-file-path-limitation?tabs=registry

The "\\?\" prefix can also be used with paths constructed according to the universal naming convention (UNC). To specify such a path using UNC, use the "\\?\UNC\" prefix. For example, "\\?\UNC\server\share", where "server" is the name of the computer and "share" is the name of the shared folder.

@yersan
Copy link
Collaborator Author

yersan commented Oct 17, 2023

Hi @bstansberry It could be a possibility and if so, we can include it here. The INST_MGR_PREPARED_SERVER_DIR used by the script is computed from the result of pathManager.getPathEntry("jboss.controller.temp.dir").resolvePath().

If the PathManager is able to return such a kind of path, then we could include the UNC here. I am going to verify this possibility first.

@yersan yersan marked this pull request as draft October 17, 2023 11:47
@yersan
Copy link
Collaborator Author

yersan commented Oct 17, 2023

It is possible to use UNC paths for configuring the temp dir, so we should allow deleting UNC paths as well. Moved to draft

@yersan yersan marked this pull request as ready for review October 17, 2023 13:11
@wildfly-ci

This comment was marked as outdated.

@darranl darranl requested a review from jamezp October 22, 2023 12:40
@darranl
Copy link
Contributor

darranl commented Oct 22, 2023

@jamezp just mentioning you as a reviewer as I think you said you could look at this one, other reviews still welcome.

@yersan
Copy link
Collaborator Author

yersan commented Oct 23, 2023

Maybe @spyrkob ? I did manual tests here and it worked on my end, so it is a matter of putting other eyes on it in case we are missing something.

@spyrkob
Copy link
Contributor

spyrkob commented Oct 23, 2023

@yersan what about Test-Path -Path $INST_MGR_PREPARED_SERVER_DIR -PathType Container and Get-ChildItem -Path $INST_MGR_PREPARED_SERVER_DIR earlier on in the script? Do they also need handle long paths?

@yersan
Copy link
Collaborator Author

yersan commented Oct 23, 2023

Hi @spyrkob , thanks for the review. It is unlikely this case could occur just with the $INST_MGR_PREPARED_SERVER_DIR and its immediate children, the issue is more probable when we have created a full server directory structure over the $INST_MGR_PREPARED_SERVER_DIR. However, the corner case could be there. I haven't tested if Test-Path -Path or Get-ChildItem -Path could hit the same error as Remove-Item. I need to check this case.

@yersan
Copy link
Collaborator Author

yersan commented Oct 23, 2023

@spyrkob I took into account your point, could you review it again?

Copy link
Contributor

@spyrkob spyrkob left a comment

Choose a reason for hiding this comment

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

Thanks @yersan, looks good as far as I can tell

@marekkopecky
Copy link
Contributor

MR LGTM, see more details on jira issue

@yersan yersan merged commit 5df90f4 into wildfly:main Oct 23, 2023
11 of 12 checks passed
@yersan yersan deleted the WFCORE-6559 branch October 23, 2023 15:18
@yersan
Copy link
Collaborator Author

yersan commented Oct 23, 2023

Thanks to all for collaborating on the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
6 participants