Skip to content

Adding option to path custom archive option #183

Merged
venmanyarun merged 2 commits intoOpenLiberty:mainfrom
venmanyarun:package_archive_path_fix
Dec 19, 2025
Merged

Adding option to path custom archive option #183
venmanyarun merged 2 commits intoOpenLiberty:mainfrom
venmanyarun:package_archive_path_fix

Conversation

@venmanyarun
Copy link
Contributor

Adding option to path custom archive option for latest versions of liberty, which support POSIX paths

Part of OpenLiberty/ci.maven#1950

…berty, which support POSIX paths

Signed-off-by: Arun Venmany <arun.kumar.v.n@ibm.com>
Copy link
Member

@cherylking cherylking left a comment

Choose a reason for hiding this comment

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

I don't agree with these changes. There is no reason to pass in a separate custom archive option. Just set a boolean saying whether to use posix rules or not, and have the logic within the existing addArchiveOption method. Also, won't LGP need the same logic? The server ant task is called from AbstractServerTask.groovy in the executeServerCommand method. The archive option is set in PackageTask.groovy in method packageServer method.

Signed-off-by: Arun Venmany <arun.kumar.v.n@ibm.com>
@venmanyarun
Copy link
Contributor Author

venmanyarun commented Dec 19, 2025

I don't agree with these changes. There is no reason to pass in a separate custom archive option. Just set a boolean saying whether to use posix rules or not, and have the logic within the existing addArchiveOption method. Also, won't LGP need the same logic? The server ant task is called from AbstractServerTask.groovy in the executeServerCommand method. The archive option is set in PackageTask.groovy in method packageServer method.

  1. Changes made based on review comments to use boolean flag
  2. LGP PR raised after adding similar tests and changes in PackageTask.groovy archive option fix for gradle ci.gradle#1029

Copy link
Member

@cherylking cherylking left a comment

Choose a reason for hiding this comment

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

lgtm

@venmanyarun venmanyarun merged commit 26379ae into OpenLiberty:main Dec 19, 2025
8 checks passed
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.

2 participants