8000 16189 removed lots of unnecessary quotes and allowed passing quoted a… by sschu · Pull Request #16229 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

16189 removed lots of unnecessary quotes and allowed passing quoted a… - #16229

Merged
pedroigor merged 2 commits into
keycloak:mainfrom
bosch-io:16189_fix_quotes_in_kc.bat
Apr 4, 2023
Merged

16189 removed lots of unnecessary quotes and allowed passing quoted a…#16229
pedroigor merged 2 commits into
keycloak:mainfrom
bosch-io:16189_fix_quotes_in_kc.bat

Conversation

@sschu
@sschu sschu commented Jan 3, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

…rgument values

Closes #16189

Also fixes #16319 for kc.bat.

@sschu
sschu force-pushed the 16189_fix_quotes_in_kc.bat branch from 01e7049 to 2eb3f86 Compare January 3, 2023 17:43
@sschu sschu mentioned this pull request Jan 3, 2023
Closed
2 tasks done
@sschu
sschu commented Jan 3, 2023
Copy link
Copy Markdown
Contributor Author

Hmm, everything green. Strange. Probably because there are no Windows test runners? @DGuhr Do you know if there are any windows test runners to test running Keycloak with kc.bat, especially the tests I changed?

@pedroigor
pedroigor commented Jan 4, 2023
Copy link
Copy Markdown
Contributor

@sschu We are not running tests in Windows. Some people in the team are working to enable it.

Looks like your changes are enough to bring more consistency between shell and bat scripts. The code you are removing from tests is an example.

If you are happy with these changes I can run some tests here and see if we can include them.

@pedroigor pedroigor self-assigned this Jan 4, 2023
@sschu
sschu commented Jan 5, 2023
Copy link
Copy Markdown
Contributor Author

@pedroigor I did a bit more local testing also checking --debug and passing system properties, this all seemed to work fine. But I guess a bit more testing is needed - if you could check that would be great. I don't have a real Windows development setup.

@pedroigor
pedroigor commented Jan 5, 2023
Copy link
Copy Markdown
Contributor

@sschu Your changes look OK and the failures I'm facing when running tests are expected:

  • HelpCommandDistTest
  • LoggingDistTest

Your changes are also fixing #16189. It should be possible to start using:

 ./kc.bat start-dev --log='"console,file"' --log-file-format='"%d{yyyy-MM-dd HH:mm:ss,SSS} %-5p [%c] (%t123) %s%e%n"'

The format is a bit different than what is described in docs.

I need some more time to fix the test failures on Windows and include an additional test to the issue you are fixing.

@sschu
sschu commented Jan 6, 2023
Copy link
Copy Markdown
Contributor Author

@pedroigor You can even leave out the outer single quotes. But you probably mean #16188 ? This can already by circumvented by adding double quotes as I commented in that issue.

@sschu sschu changed the title [WIP] 16189 removed lots of unnecessary quotes and allowed passing quoted a… 16189 removed lots of unnecessary quotes and allowed passing quoted a… Jan 9, 2023 8000
@sschu sschu mentioned this pull request Jan 9, 2023
Closed
2 tasks done
@pedroigor
pedroigor commented Jan 9, 2023
Copy link
Copy Markdown
Contributor

@sschu So the changes here are basically improvements to code? I mean, we can close #16189 and #16188?

@sschu
sschu commented Jan 9, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@pedroigor This one here closes #16189 completely and #16319 for kc.bat. To close #16319 for kc.sh there is #16339.
#16188 already works if you quote the log handlers correctly, I updated the documentation for that: #16298.

@mabartos
mabartos commented Mar 8, 2023
Copy link
Copy Markdown
Member

Any progress here?

@Pepo48 Could you please check it? Thanks

@Pepo48
Pepo48 commented Mar 8, 2023
Copy link
Copy Markdown
Contributor

@mabartos I can check this one on my local machine tomorrow. Regardless, we can run this even in our internal pipelines just to be completely sure, right?

Anyway, from my point of view the changes LGTM.

pedroigor
pedroigor previously approved these changes Mar 27, 2023
View reviewed changes
@sschu
sschu requested review from a team as code owners April 4, 2023 09:08
@ghost ghost added team/cloud-native labels Apr 4, 2023
@sschu
sschu commented Apr 4, 2023
Copy link
Copy Markdown
Contributor Author

I just resolved conflicts.

@pedroigor
pedroigor merged commit 1c9992f into keycloak:main Apr 4, 2023
@Pepo48 Pepo48 mentioned this pull request Apr 15, 2023
Merged
@Eng-Fouad
Eng-Fouad commented Apr 19, 2023
Copy link
Copy Markdown
Contributor

See #19803

@ahus1 ahus1 mentioned this pull request Jul 6, 2023
Closed
@Pepo48 Pepo48 mentioned this pull request Oct 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pedroigor pedroigor pedroigor approved these changes

Assignees

@pedroigor pedroigor

Labels

team/cloud-native

Projects

None yet

Milestone

No milestone

5 participants

@sschu @pedroigor @mabartos @Pepo48 @Eng-Fouad
0