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-6895] Add a grace period to Management CLI Installer Windows … #6071

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

yersan
Copy link
Collaborator

@yersan yersan commented Jul 15, 2024

…script

Jira issue: https://issues.redhat.com/browse/WFCORE-6895

This just adds a configurable grace period to help prevent locking file errors on Windows when using Management CLI Installer

@yersan yersan requested a review from spyrkob July 15, 2024 09:12
@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 15, 2024
@@ -89,3 +89,6 @@ if ($HOST_CONTROLLER_JAVA_OPTS -eq $null) {
# Sample JPDA settings for shared memory debugging
#PROCESS_CONTROLLER_JAVA_OPTS="$PROCESS_CONTROLLER_JAVA_OPTS -agentlib:jdwp=transport=dt_shmem,server=y,suspend=n,address=jboss"
#HOST_CONTROLLER_JAVA_OPTS="$HOST_CONTROLLER_JAVA_OPTS -agentlib:jdwp=transport=dt_shmem,server=y,suspend=n,address=jboss"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not connected to this PR, but should those assignments start with $sign (e.g. $HOST_CONTROLLER_JAVA_OPTS)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!, variables in Powershell are defined starting the with $, I'll append a commit

@@ -177,11 +176,6 @@ protected void doHandle(CommandContext ctx) throws CommandLineException {

if(isLocalClient) {
ctx.printLine("The JBoss CLI session will be closed automatically to allow the server be updated. Once the server has been restarted, you can relaunch the JBoss CLI session.", false);
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

The wait in the startup scripts only applies to Windows scripts, but this wait happened also on Linux - is that OK to remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@spyrkob thanks for commenting on it. It does nothing, it just waits a bit to have a better UX; you execute the command and see how it waits a bit with the message printed, that's all. It is safe to close it immediately.

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.

Couple of questions below, otherwise LGTM

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Jul 18, 2024
@yersan yersan merged commit b116db7 into wildfly:main Jul 18, 2024
10 of 12 checks passed
@yersan yersan deleted the WFCORE-6895 branch July 18, 2024 08:29
@yersan
Copy link
Collaborator Author

yersan commented Jul 18, 2024

Thanks @spyrkob

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 ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants