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.

Sign up for GitHub

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

Jump to bottom

migrate to jhipster v8 #296

Merged
merged 30 commits into from
Nov 3, 2023
Merged

migrate to jhipster v8 #296

merged 30 commits into from
Nov 3, 2023

Conversation

mshima
Copy link
Member

@mshima mshima commented Oct 31, 2023 •
edited
Loading

Fixes #294

@mshima
Copy link
Member Author

mshima commented Nov 1, 2023

The remaining failure is related to keycloak 19 -> 22:

2023-11-01 03:42:56,873 INFO  [tc.qua.io/.0.5] (docker-java-stream--304219373) STDOUT: 2023-11-01 03:42:56,871 WARN  [org.keycloak.events] (executor-thread-1) type=LOGIN_ERROR, realmId=jhipster, clientId=web_app, userId=null, ipAddress=172.17.0.1, error=not_allowed, auth_method=oauth_credentials, grant_type=password, client_auth_method=client-secret
2023-11-01 03:42:57,066 INFO  [http-problem] (executor-thread-1) status=401, title="Unauthorized"
Error:  Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 88.54 s <<< FAILURE! -- in io.github.jhipster.sample.web.rest.AccountResourceIT
Error:  io.github.jhipster.sample.web.rest.AccountResourceIT.should_allow_authenticated_user_to_retrieve_own_account_details -- Time elapsed: 2.008 s <<< FAILURE!
java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <400>.

generator-jhipster-quarkus/generators/server/templates/quarkus/src/test/java/package/web/rest/AccountResourceIT_oauth2.java.ejs

Lines 33 to 46 in 2abf8d8

void should_allow_authenticated_user_to_retrieve_own_account_details() {
given()
.auth().oauth2(getAccessToken("admin", "admin"))
.accept(APPLICATION_JSON)
.when()
.get("/account")
.then()
.statusCode(OK.getStatusCode())
.contentType(APPLICATION_JSON)
.body("login", is("admin"))
.body("firstName", is("Admin"))
.body("lastName", is("Administrator"))
.body("email", is("admin@localhost"))
.body("authorities", hasItems(AuthoritiesConstants.USER, AuthoritiesConstants.ADMIN));

Maybe related to legacy grant_type=password?
The generated backend code is identical (almost, v7 -> v8 changes like package-info auto generation, some relationship changes, etc).

@mraible not sure what's the fix.

@mraible
Copy link
Contributor

mraible commented Nov 1, 2023

@mshima Do you happen to have steps to reproduce? If we're using grant_type=password in code, we should fix that. It bypasses the whole point of OAuth, IMHO.

@mraible
Copy link
Contributor

mraible commented Nov 1, 2023

@vishal423 Do you have any suggestions?

@mshima
Copy link
Member Author

mshima commented Nov 1, 2023 •
edited
Loading

@mraible that test seems to login at keycloak and gets the account details from '/account' endpoint with that authentication token.
This test probably would require a reimplementation to drop grant_type=password and simulate the entire authentication workflow.
Or grant_type=password should be enabled at the keycloak test container.
IMO the test should be dropped and /account should be tested by others means.

I can revert keycloak testcontainer to v19 and use copy the keycloak realm from v7.9.4 to use at the test.
What do you think?

@mraible
Copy link
Contributor

mraible commented Nov 1, 2023

I can revert keycloak testcontainer to v19 and use copy the keycloak realm from v7.9.4 to use at the test.

I think this is a good way to test if the Keycloak configuration has changed.

IMO the test should be dropped and /account should be tested by others means.

I agree since it's not testing functionality that's used in a JHipster-generated app.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 2, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 6 Code Smells

No Coverage information No Coverage information
1.6% 1.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@mshima
Copy link
Member Author

mshima commented Nov 2, 2023

@mraible tests are passing now.
I need to do more adjusts post merged adjust like change the diff logic and update to node 18.

mraible
mraible reviewed Nov 2, 2023
View reviewed changes
.blueprint/generate-sample/generator.mjs
import command from './command.mjs';

export default class extends BaseGenerator {
sampleName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 2-space indents instead of 4?

Copy link
Member Author

Choose a reason for hiding this comment

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

prettier config

mraible
mraible reviewed Nov 2, 2023
View reviewed changes
.blueprint/generate-sample/templates/samples/imperative-ngx-jwt-maven-mysql-redis.jdl
@@ -4,6 +4,7 @@ application {
authenticationType jwt
baseName jhipsterSampleApplication
buildTool maven
creationTimestamp 1617901618891
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? I'm guessing it's not a required field.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes the diff in CI compare step smaller.

mraible
mraible reviewed Nov 2, 2023
View reviewed changes
.github/workflows/smoke-test-imperative.yml
@@ -53,20 +57,21 @@ jobs:
uses: actions/checkout@v3
with:
repository: 'jhipster/generator-jhipster'
ref: v7.9.3
ref: v8.0.0-rc.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 8.0.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try to drop in the following PR.

mraible
mraible reviewed Nov 2, 2023
View reviewed changes
.prettierrc
}]
"printWidth": 140,
"singleQuote": true,
"tabWidth": 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this set the spaces to 4 instead of 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is the blueprint old configuration.
I didn't change.

generator-jhipster-quarkus/.prettierrc

Line 4 in 2abf8d8

"tabWidth": 4,

Copy link
Member Author

Choose a reason for hiding this comment

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

It's up to the maintainer to decide indentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's quite easy to change. Change this file and run npx prettier . --write.

mraible
mraible reviewed Nov 2, 2023
View reviewed changes
.yo-rc.json
@@ -1,10 +1,126 @@
{
"generator-jhipster": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be indented 2 spaces instead of 4?

Copy link
Member Author

Choose a reason for hiding this comment

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

prettier config.

@mraible mraible merged commit 71e9c1b into jhipster:main Nov 3, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraible mraible mraible approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Release v3.0 with Quarkus 3 support
2 participants
@mshima @mraible