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

Propagate Quarkus Redis Config to Vertx Redis Client #17714

Merged
merged 1 commit into from
Jun 6, 2021
Merged

Propagate Quarkus Redis Config to Vertx Redis Client #17714

merged 1 commit into from
Jun 6, 2021

Conversation

antoniodvr
Copy link
Contributor

@antoniodvr antoniodvr commented Jun 5, 2021 •
edited
Loading

Fixes: #17673

@antoniodvr antoniodvr changed the title Propagate Quarkus Redis Config to Vertx Redis Client 17673 Propagate Quarkus Redis Config to Vertx Redis Client Jun 5, 2021
@antoniodvr antoniodvr changed the title 17673 Propagate Quarkus Redis Config to Vertx Redis Client 17673 - Propagate Quarkus Redis Config to Vertx Redis Client Jun 5, 2021
@machi1990 machi1990 self-requested a review June 6, 2021 09:43
machi1990
machi1990 requested changes Jun 6, 2021
View reviewed changes
Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

@antoniodvr thanks for the PR. This is really good and even better than what I was initially thinking of i.e exposing only one certificate format. I've added some suggestions around to remove manually initialising some configuration knobs as the Quarkus config will do it for us.

Other than that, I've asked for two tcp options to be also exposed as secrets.

extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java Outdated Show resolved Hide resolved
extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java Outdated Show resolved Hide resolved
@machi1990 machi1990 marked this pull request as ready for review June 6, 2021 10:21
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 07ee3d8

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 36432d1

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Set up job ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 2546818

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Run actions/checkout@v2 ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 84110f2

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Set up job ⚠️ Check → Logs Raw logs
⌛ Attach pull request number Set up job ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 76b20df

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Set up job ⚠️ Check → Logs Raw logs
⌛ Attach pull request number Set up job ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 3fc7bec

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build ⚠️ Check → Logs Raw logs
⌛ Attach pull request number ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 72e67cd

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Set up job ⚠️ Check → Logs Raw logs
⌛ Attach pull request number Set up job ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building abb7344

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build ⚠️ Check → Logs Raw logs
⌛ Attach pull request number ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 941f1b3

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Set up job ⚠️ Check → Logs Raw logs
⌛ Attach pull request number Set up job ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 39a27b8

Status Name Step Test failures Logs Raw logs
✖ Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@machi1990
Copy link
Member

Thanks @antoniodvr for taking care of suggested changes, great change and improvement, great work. Do you mind squashing the commits? Once done, we'll wait for CI to pass and merge.

@antoniodvr
Propagate Quarkus Redis Config to Vertx Redis Client
6922495 
Propagate Quarkus Redis Config to Vertx Redis Client

Propagate Quarkus Redis Config to Vertx Redis Client

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/RedisConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Update extensions/redis-client/runtime/src/main/java/io/quarkus/redis/client/runtime/SslConfig.java

Co-authored-by: Manyanda Chitimbo <[email protected]>

Peer Review changes
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building fdcde21

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

machi1990
machi1990 approved these changes Jun 6, 2021
View reviewed changes
Copy link
Member

@machi1990 machi1990 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Great work, thanks @antoniodvr!

@machi1990 machi1990 added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 6, 2021
@antoniodvr
Copy link
Contributor Author

Thanks @antoniodvr for taking care of suggested changes, great change and improvement, great work. Do you mind squashing the commits? Once done, we'll wait for CI to pass and merge.

Thanks to you! Commits squashed.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 6922495

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
⌛ JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✖ JVM Tests - JDK 16 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 extensions/vertx-http/deployment

✖ io.quarkus.vertx.http.testrunner.TestRunnerSmokeTestCase.checkTestsAreRun line 48 - More details - Source on GitHub

@cescoffier cescoffier merged commit f07b67b into quarkusio:2.0 Jun 6, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 6, 2021
@cescoffier cescoffier mentioned this pull request Jun 7, 2021
Closed
@gsmet gsmet changed the title 17673 - Propagate Quarkus Redis Config to Vertx Redis Client Propagate Quarkus Redis Config to Vertx Redis Client Jun 7, 2021
@gsmet
Copy link
Member

gsmet commented Jun 7, 2021

Hmmm. This PR was targeting 2.0. I'm not sure that's what we wanted? Has this been merged in main before? /cc @machi1990

@machi1990
Copy link
Member

Hmmm. This PR was targeting 2.0. I'm not sure that's what we wanted? Has this been merged in main before? /cc @machi1990

@gsmet good catch. I am sorry, I should have seen the target branch before this was merged. No, I do not believe it is in main branch. I'll double check and cherry-pick this if it is not the case.

@machi1990 machi1990 mentioned this pull request Jun 7, 2021
Merged
@machi1990
Copy link
Member

Hmmm. This PR was targeting 2.0. I'm not sure that's what we wanted? Has this been merged in main before? /cc @machi1990

@gsmet good catch. I am sorry, I should have seen the target branch before this was merged. No, I do not believe it is in main branch. I'll double check and cherry-pick this if it is not the case.

I've opened #17738 to merge it into master.

@gsmet gsmet added this to the 2.0.0.Final milestone Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machi1990 machi1990 machi1990 approved these changes

Assignees
No one assigned
Labels
area/redis
Projects
None yet
Milestone
2.0.0.Final
Development

Successfully merging this pull request may close these issues.
4 participants
@antoniodvr @machi1990 @gsmet @cescoffier