8000 Improve quoting and variable parsing for batch start script. by jsorah · Pull Request #20212 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Improve quoting and variable parsing for batch start script. - #20212

Merged
vmuzikar merged 1 commit into
keycloak:mainfrom
jsorah:20115-fix-batch-quoting-and-variable-parsing-issues
Oct 20, 2023
Merged

Improve quoting and variable parsing for batch start script.#20212
vmuzikar merged 1 commit into
keycloak:mainfrom
jsorah:20115-fix-batch-quoting-and-variable-parsing-issues

Conversation

@jsorah
@jsorah jsorah commented May 8, 2023
Copy link
Copy Markdown
Contributor

Quote some specific variables used in the batch script to address blank space issues when Keycloak is in a directory structure containing spaces.

Address some variable parsing issues by using delayed expansion instead of default behavior.

Remove superfluous SetLocal EnableDelayedExpansion

Closes #20115

@jsorah
jsorah requested review from a team as code owners May 8, 2023 12:20
@ghost ghost added team/cloud-native labels May 8, 2023
@jsorah jsorah mentioned this pull request May 8, 2023
Closed
2 tasks done
@jsorah
jsorah commented May 8, 2023
Copy link
Copy Markdown
Contributor Author

@miquelsi I was told there were some additional checks you could run for this PR specific to Windows systems?

@jsorah
jsorah force-pushed the 20115-fix-batch-quoting-and-variable-parsing-issues branch from 356cde4 to e8c65db Compare May 8, 2023 18:21
@vmuzikar
vmuzikar requested a review from Pepo48 May 9, 2023 06:33
@jsorah
jsorah force-pushed the 20115-fix-batch-quoting-and-variable-parsing-issues branch from e8c65db to de640b6 Compare May 9, 2023 12:47
@ghost ghost added the flaky-test label May 9, 2023
@ghost ghost mentioned this pull request May 9, 2023
Closed
ghost
ghost reviewed May 9, 2023
View reviewed changes
@ghost ghost left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
ghost commented May 9, 2023
Copy link
Copy Markdown

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.model.infinispan.CacheExpirationTest#testCacheExpiration

Keycloak CI - Store Model Tests

java.lang.AssertionError: 
threads didn't terminate in time: [main (RUNNABLE):
	at java.management@17.0.6/sun.management.ThreadImpl.dumpThreads0(Native Method)
	at java.management@17.0.6/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:521)
	at java.management@17.0.6/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:509)
...

Report flaky test

org.keycloak.testsuite.model.infinispan.CacheExpirationTest#testCacheExpiration

Keycloak CI - Store Model Tests

java.lang.AssertionError: 
threads didn't terminate in time: [main (RUNNABLE):
	at java.management@17.0.6/sun.management.ThreadImpl.dumpThreads0(Native Method)
	at java.management@17.0.6/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:521)
	at java.management@17.0.6/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:509)
...

Report flaky test

@jsorah jsorah mentioned this pull request Jun 22, 2023
Closed
2 tasks done
@jsorah
jsorah commented Jun 22, 2023
Copy link
Copy Markdown
Contributor Author

@Pepo48 - I see @vmuzikar moved the original issue out of kc22 milestone - whats the best way to move this PR forward at this point? I just don't want to leave this hanging open indefinitely.

@vmuzikar
vmuzikar commented Jun 26, 2023
Copy link
Copy Markdown
Contributor

@jsorah Unfortunately, due to other priorities we're unable to prioritize any non-critical Windows issues for 22. If we find some time for a review, we can merge. :)

CC @Pepo48

@jsorah
jsorah force-pushed the 20115-fix-batch-quoting-and-variable-parsing-issues branch 2 times, most recently from e246601 to 338b613 Compare July 3, 2023 14:18
This was referenced Jul 3, 2023
Closed
Closed
@sschu sschu mentioned this pull request Jul 4, 2023
Closed
2 tasks done
@jsorah
jsorah force-pushed the 20115-fix-batch-quoting-and-variable-parsing-issues branch from 338b613 to de38b7c Compare July 5, 2023 18:55
@ghost ghost mentioned this pull request Jul 5, 2023
Closed
ghost
ghost reviewed Jul 5, 2023
View reviewed changes
@ghost ghost left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
ghost commented Jul 5, 2023
Copy link
Copy Markdown

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionAtSameNode

Keycloak CI - Store Model Tests

java.lang.AssertionError: 
threads didn't terminate in time: [main (TIMED_WAITING): locked on java.util.concurrent.FutureTask@39fcbdfa waiting for 137222 ms blocked for 18130 ms
	at java.base@17.0.7/jdk.internal.misc.Unsafe.park(Native Method)
	at java.base@17.0.7/java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:252)
	at java.base@17.0.7/java.util.concurrent.FutureTask.awaitDone(FutureTask.java:444)
...

Report flaky test

@Pepo48
Pepo48 commented Aug 10, 2023
Copy link
Copy Markdown
Contributor

@jsorah can you do the rebase, please? We can then check the results of our new Windows workflow in GH CI.

Thanks!

@jsorah
jsorah force-pushed the 20115-fix-batch-quoting-and-variable-parsing-issues branch from de38b7c to 5a75f21 Compare August 10, 2023 12:02
@jsorah
jsorah commented Aug 10, 2023
Copy link
Copy Markdown
Contributor Author

@jsorah can you do the rebase, please? We can then check the results of our new Windows workflow in GH CI.

Thanks!

@Pepo48 Done!

@Pepo48
Pepo48 commented Aug 28, 2023
Copy link
Copy Markdown
Contributor

@jsorah this PR LGTM. Thanks!

@miquelsi can QE test this internally?

@Pepo48 Pepo48 mentioned this pull request Oct 19, 2023
Closed
@vmuzikar vmuzikar linked an issue Oct 20, 2023 that may be closed by this pull request
Can't start Keycloak 22.0.3 on Windows #23563
Closed
2 tasks done
@vmuzikar
vmuzikar commented Oct 20, 2023
Copy link
Copy Markdown
Contributor

I'd like to get back to this PR, @jsorah sorry for the long wait. :) Could you please rebase to resolve conflicts?

@jsorah
Improve quoting and variable parsing for batch start script.
81fb2bd 
Quote some specific variables used in the batch script to address blank
space issues when Keycloak is in a directory structure containing
spaces.

Address some variable parsing issues by using delayed expansion instead
of default behavior.

Remove superfluous SetLocal EnableDelayedExpansion

Closes keycloak#20115
@jsorah
jsorah force-pushed the 20115-fix-batch-quoting-and-variable-parsing-issues branch from 5a75f21 to 81fb2bd Compare October 20, 2023 14:37
@jsorah
jsorah commented Oct 20, 2023
Copy link
Copy Markdown
Contributor Author

@vmuzikar - Done! Looks like some similar changes landed in main, this PR still does fix a couple of quoting bits too.

@vmuzikar
vmuzikar commented Oct 20, 2023
Copy link
Copy Markdown
Contributor

@jsorah Thanks!

@Pepo48 Can you please re-review?

Pepo48
Pepo48 approved these changes Oct 20, 2023
View reviewed changes
@Pepo48 Pepo48 left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

vmuzikar
vmuzikar approved these changes Oct 20, 2023
View reviewed changes
@vmuzikar vmuzikar left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving based on @Pepo48's review.

Thanks!

@vmuzikar
vmuzikar enabled auto-merge (squash) October 20, 2023 14:53
@vmuzikar
vmuzikar merged commit 1837b44 into keycloak:main Oct 20, 2023
@jsorah jsorah mentioned this pull request Nov 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pepo48 Pepo48 Pepo48 approved these changes
@vmuzikar vmuzikar vmuzikar approved these changes
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

@Pepo48 Pepo48

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't start Keycloak 22.0.3 on Windows Windows: keycloak-21.1.1 start up error

3 participants

@jsorah @vmuzikar @Pepo48
0