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.

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

Add option to start Redis Dev Services on Null Host Value #41128

Closed
The-Funk opened this issue Jun 11, 2024 · 17 comments · Fixed by #41326
Closed

Add option to start Redis Dev Services on Null Host Value #41128

The-Funk opened this issue Jun 11, 2024 · 17 comments · Fixed by #41326

Comments

@The-Funk
Copy link

The-Funk commented Jun 11, 2024

Description

Some of my build processes occur inside containers in a staging Kubernetes environment. Dev services containers cannot be started in this environment.

On my normal development laptop however, dev services can be used to provide a Redis instance, a SQL Server, etc, nice!

One thing that I like about how certain extensions have implemented dev services, is the option to have them start if the provided hostname or host is null or empty.

This makes dev services a little more flexible on a per profile basis, which has allowed me to run my local tests using the same testing profile (%test) as my tests on my build containers in my staging environment.

Would it be possible to also add an option in the Redis extension to start dev services for Redis under a given set of conditions in a given profile?

In my case, I would want dev services to start in the dev profile always but in the test profile, if and only if the Redis hosts property is null/empty according to the smallrye config.

Implementation ideas

No response

Copy link

quarkus-bot bot commented Jun 11, 2024

/cc @Ladicek (redis), @cescoffier (redis), @geoand (devservices,kubernetes), @iocanel (kubernetes), @machi1990 (redis), @stuartwdouglas (devservices)

@cescoffier
Copy link
Member

That should be the case already no? If the redis host is not configured, it should start the dev service.

Or, do you mean you need the dev service when the programmatic host resolver returns null too. In that case, this is too late to start a dev service. The application is already running.

@The-Funk
Copy link
Author

The-Funk commented Jun 11, 2024

@cescoffier I found the line and forked the Quarkus repo.

This line says if the config property is present, don't start dev services. In my case, the property is present but pointed at an environment variable on the system, which may or may not be null. On my laptop, the environment variable is not set, so I would expect the dev services to start, however they do not. In my build server, because I cannot use containers, there is an instance of redis running that I can point to using the environment variable.

Here's what's in my application properties

quarkus.redis.hosts=${REDIS_URL}

If the value is null, I'd like dev services to start, if the value is not null, I'd like dev services to not start.

I understand this might represent a change in behavior, so I imagine there would need to be another flag to gate the changed behavior. Something along the lines of:

if (redis-devservices-start-when-null-host && host-is-null) {
   needStart = true;
}

@cescoffier
Copy link
Member

I see, it's not null. An environment variable cannot be defined and null. It can be empty.

We can check for empty, and start the dev service in that case.

Fancy a PR? You identified the line already.

@cescoffier
Copy link
Member

Well we should check for null and empty, just in case.

@The-Funk
Copy link
Author

@cescoffier I don't mind putting a PR in a little later this evening, it would be my first PR to Quarkus though, so go easy if I break some conventions.

@The-Funk
Copy link
Author

If the change is acceptable to be made to the LTS branch, would I just open a separate PR based on the 3.8 branch?

@geoand
Copy link
Contributor

geoand commented Jun 11, 2024

No need, we backport automatically (if there is agreement to bring the change to LTS)

@The-Funk
Copy link
Author

No need, we backport automatically (if there is agreement to bring the change to LTS)

I saw this a moment too late and ended up submitting two PRs.

@ozangunalp
Copy link
Contributor

This is the case for almost every dev service by the way. Changing only for Redis would create inconsistency.

@cescoffier
Copy link
Member

That was I was thinking. But it seems that some dev services check for null or empty.

We would need to check which dev service does this.

@The-Funk
Copy link
Author

The-Funk commented Jun 12, 2024

This is the case for almost every dev service by the way. Changing only for Redis would create inconsistency.

Are you saying most extensions only start dev services on missing config property or that most start on missing or empty config property?

I think missing-or-empty is a more flexible configuration, I don't mind bringing that change to other extensions as well if consistency is important.

@gsmet
Copy link
Member

gsmet commented Jun 12, 2024

FWIW, I'm all for the change that was made in the PR and I think we should generalize it to all Dev Services checking a URL (or something similar).

@ozangunalp
Copy link
Contributor

This is the case for almost every dev service by the way. Changing only for Redis would create inconsistency.

Are you saying most extensions only start dev services on missing config property or that most start on missing or empty config property?

I think missing-or-empty is a more flexible configuration, I don't mind bringing that change to other extensions as well if consistency is important.

Most dev services use an utility method which checks for missing-or-empty, but without expression expansion.
If we go forward with this we can change / add a new method to enable expression expansion.

@The-Funk
Copy link
Author

This is the case for almost every dev service by the way. Changing only for Redis would create inconsistency.

Are you saying most extensions only start dev services on missing config property or that most start on missing or empty config property?
I think missing-or-empty is a more flexible configuration, I don't mind bringing that change to other extensions as well if consistency is important.

Most dev services use an utility method which checks for missing-or-empty, but without expression expansion. If we go forward with this we can change / add a new method to enable expression expansion.

That would be super helpful for use cases like my own, where I want to test using a consistent profile on my local machine and my build servers, but only have access to a container runtime in one of those two environments.

@The-Funk
Copy link
Author

I can't seem to get the maven formatter to run on my system, but if you all would like, I can submit a separate PR later tonight to add that new method to the ConfigUtility.

gsmet pushed a commit to ozangunalp/quarkus that referenced this issue Jun 24, 2024
ozangunalp added a commit to ozangunalp/quarkus that referenced this issue Jun 27, 2024
Removed self-referencing property expansion in oidc integration test

Fixes quarkusio#41128
ozangunalp added a commit to ozangunalp/quarkus that referenced this issue Jun 27, 2024
Removed self-referencing property expansion in oidc integration test

Fixes quarkusio#41128
ozangunalp added a commit to ozangunalp/quarkus that referenced this issue Jun 27, 2024
Removed self-referencing property expansion in oidc integration test

Fixes quarkusio#41128
@cescoffier
Copy link
Member

@ozangunalp what's the status on this one?

ozangunalp added a commit to ozangunalp/quarkus that referenced this issue Jul 1, 2024
Removed self-referencing property expansion in oidc integration test

Fixes quarkusio#41128
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 2, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
Removed self-referencing property expansion in oidc integration test

Fixes quarkusio#41128
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
Removed self-referencing property expansion in oidc integration test

Fixes quarkusio#41128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment