Skip to content

Use Java mechanisms to read language files and default to UTF-8 - #21755

Merged
mhajas merged 1 commit into
keycloak:mainfrom
ahus1:is-21753-standardize-translation-handling
Aug 1, 2023
Merged

mhajas merged 1 commit into
keycloak:mainfrom
ahus1:is-21753-standardize-translation-handling

Conversation

@ahus1

@ahus1 ahus1 commented Jul 17, 2023

Copy link
Copy Markdown
Member

Closes #21753

Discussion: #9270

@ahus1

ahus1 commented Jul 17, 2023

Copy link
Copy Markdown
Member Author

While this is still in draft, it already contains updates to all property files and the docs.

@ahus1 ahus1 self-assigned this Jul 17, 2023
jonkoops
jonkoops previously approved these changes Jul 17, 2023

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression

This path depends on a [user-provided value](1). This path depends on a [user-provided value](2). This path depends on a [user-provided value](3). This path depends on a [user-provided value](4). This path depends on a [user-provided value](5). This path depends on a [user-provided value](6). This path depends on a [user-provided value](7). This path depends on a [user-provided value](8). This path depends on a [user-provided value](9). This path depends on a [user-provided value](10). This path depends on a [user-provided value](11). This path depends on a [user-provided value](12). This path depends on a [user-provided value](13). This path depends on a [user-provided value](14). This path depends on a [user-provided value](15). This path depends on a [user-provided value](16). This path depends on a [user-provided value](17). This path depends on a [user-provided value](18). This path depends on a [user-provided value](19). This path depends on a [user-provided value](20). This path depends on a [user-provided value](21). This path depends on a [user-provided value](22). This path depends on a [user-provided value](23). This path depends on a [user-provided value](24). This path depends on a [user-provided value](25). This path depends on a [user-provided value](26). This path depends on a [user-provided value](27). This path depends on a [user-provided value](28). This path depends on a [user-provided value](29). This path depends on a [user-provided value](30). This path depends on a [user-provided value](31). This path depends on a [user-provided value](32). This path depends on a [user-provided value](33). This path depends on a [user-provided value](34). This path depends on a [user-provided value](35). This path depends on a [user-provided value](36). This path depends on a [user-provided value](37). This path depends on a [user-provided value](38). This path depends on a [user-provided value](39). This path depends on a [user-provided value](40). This path depends on a [user-provided value](41). This path depends on a [user-provided value](42). This path depends on a [user-provided value](43). This path depends on a [user-provided value](44). This path depends on a [user-provided value](45). This path depends on a [user-provided value](46). This path depends on a [user-provided value](47). This path depends on a [user-provided value](48). This path depends on a [user-provided value](49). This path depends on a [user-provided value](50). This path depends on a [user-provided value](51). This path depends on a [user-provided value](52). This path depends on a [user-provided value](53). This path depends on a [user-provided value](54). This path depends on a [user-provided value](55). This path depends on a [user-provided value](56). This path depends on a [user-provided value](57). This path depends on a [user-provided value](58). This path depends on a [user-provided value](59). This path depends on a [user-provided value](60). This path depends on a [user-provided value](61). This path depends on a [user-provided value](62). This path depends on a [user-provided value](63). This path depends on a [user-provided value](64). This path depends on a [user-provided value](65). This path depends on a [user-provided value](66). This path depends on a [user-provided value](67). This path depends on a [user-provided value](68). This path depends on a [user-provided value](69). This path depends on a [user-provided value](70). This path depends on a [user-provided value](71). This path depends on a [user-provided value](72). This path depends on a [user-provided value](73). This path depends on a [user-provided value](74).
@ahus1

ahus1 commented Jul 18, 2023

Copy link
Copy Markdown
Member Author

It would be great to have a review from one of the maintainers on the Java side on this. Thanks!

@hmlnarik hmlnarik 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 for the Java part

hmlnarik
hmlnarik previously approved these changes Jul 19, 2023
@jonkoops

Copy link
Copy Markdown
Contributor

@ahus1 looks like the code scanner has some issues with the changes introduced here.

@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 20, 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.forms.LoginTest#loginDifferentUserAfterDisabledUserThrownOut

Keycloak CI - Forms IT (firefox)

java.lang.AssertionError: Expected LoginPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at org.keycloak.testsuite.pages.LoginPage.open(LoginPage.java:236)
...

Report flaky test

1 similar comment
@ghost

ghost commented Jul 20, 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.forms.LoginTest#loginDifferentUserAfterDisabledUserThrownOut

Keycloak CI - Forms IT (firefox)

java.lang.AssertionError: Expected LoginPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at org.keycloak.testsuite.pages.LoginPage.open(LoginPage.java:236)
...

Report flaky test

@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

@ahus1

ahus1 commented Jul 22, 2023

Copy link
Copy Markdown
Member Author

@jonkoops - I've look at the CodeQL alert, and also added a Regex check for filenames - CodeQL still complains. The logic to create the filename didn't change in this PR, so I assume it is as safe as before. I'll follow up with Bruno to find out why CodeQL complains here.

@jonkoops

Copy link
Copy Markdown
Contributor

@ahus1 sounds good to me, should we go ahead and merge this then?

@ahus1

ahus1 commented Jul 22, 2023

Copy link
Copy Markdown
Member Author

@jonkoops - yes, I think this is the way to go. Please approve it for the UI part.
I'll also get an re-approval of a maintainer/Java developer, and then it will be ready to be merged.

jonkoops
jonkoops previously approved these changes Jul 22, 2023

@jonkoops jonkoops 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

@ahus1

ahus1 commented Jul 23, 2023

Copy link
Copy Markdown
Member Author

@hmlnarik - I've added one more commit to make CodeQL happy, but it continues to warn. I'd like to keep that change, and have it squashed when committing. I'll contact @abstractj separately to figure out how to make CodeQL happy.

Could you please re-approve and merge? Thanks!

@ahus1
ahus1 force-pushed the is-21753-standardize-translation-handling branch from 471daad to e7c31c1 Compare July 26, 2023 07:42
@ahus1

ahus1 commented Jul 26, 2023

Copy link
Copy Markdown
Member Author

Rebased to resolve conflicts on the docs. Also squashed the two commits.

@mhajas mhajas 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.

thank you for the PR @ahus1, I added two comments. Also, it seems a rebase is needed (sorry for the late review).

Comment thread docs/documentation/release_notes/topics/23_0_0.adoc Outdated
Comment thread docs/documentation/upgrading/topics/keycloak/changes-23_0_0.adoc Outdated
@ahus1
ahus1 force-pushed the is-21753-standardize-translation-handling branch from e7c31c1 to badab44 Compare July 31, 2023 10:05
@ahus1
ahus1 force-pushed the is-21753-standardize-translation-handling branch from badab44 to 21eda42 Compare July 31, 2023 10:05
@ahus1
ahus1 requested a review from mhajas July 31, 2023 10:07

@mhajas mhajas 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.

Thank you @ahus1

@mhajas
mhajas enabled auto-merge (squash) July 31, 2023 11:27
@ahus1

ahus1 commented Jul 31, 2023

Copy link
Copy Markdown
Member Author

@jonkoops - could you please re-review before we run into another merge conflict? Thank you very much!

@ahus1
ahus1 requested a review from jonkoops July 31, 2023 11:56
@mhajas
mhajas merged commit 748c53d into keycloak:main Aug 1, 2023
@ahus1
ahus1 deleted the is-21753-standardize-translation-handling branch September 21, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardize message property handling for translation tools

5 participants