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

Standardize configuration properties #4452

Merged
merged 13 commits into from
Apr 29, 2024
Merged

Standardize configuration properties #4452

merged 13 commits into from
Apr 29, 2024

Conversation

carlesarnal
Copy link
Member

@carlesarnal carlesarnal commented Mar 18, 2024 •
edited
Loading

The PR finalizes the process of standardising the configuration properties for Registry, following the main guidelines:

  • No environment variable redefinition, there's only one way for configuring a value.
  • All Apicurio related env vars are runtime by default.
  • Quarkus values are use whenever possible.

There's one missing thing, for upgrades, create a table with the old values and the new ones that we can add to the migration guide from 2.x to 3.x. I'll start working on that.

@carlesarnal carlesarnal force-pushed the standardize-configuration-properties branch 5 times, most recently from 4bc4506 to a23e7a5 Compare March 28, 2024 13:41
@carlesarnal carlesarnal force-pushed the standardize-configuration-properties branch 2 times, most recently from 7415f49 to 1d44363 Compare April 2, 2024 17:15
@carlesarnal carlesarnal requested a review from EricWittmann April 2, 2024 17:36
@carlesarnal carlesarnal linked an issue Apr 9, 2024 that may be closed by this pull request
Improve client credential server caching #4233
Closed
EricWittmann
EricWittmann reviewed Apr 10, 2024
View reviewed changes
app/src/main/java/io/apicurio/registry/auth/AuthConfig.java
@@ -17,7 +17,7 @@ public class AuthConfig {
@Inject
Logger log;

@ConfigProperty(name = "registry.auth.enabled", defaultValue = "false")
@ConfigProperty(name = "quarkus.oidc.tenant-enabled", defaultValue = "false")
Copy link
Member

Choose a reason for hiding this comment

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

Should we be documenting the quarkus properties we use in this way via @Info?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, I decided not to because we can only annotate the ones we're currently injecting. For example, this means we can annotate quarkus.oidc.tenant-enabled but no quarkus.oidc.client-id. We can of course inject them even if we're not directly using them, but I don't really like that solution.

Copy link
Member

Choose a reason for hiding this comment

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

Well it would be nice to have all commonly used properties in the documentation, however we can manage to do that.

@EricWittmann
Copy link
Member

Don't we also want to rename all registry. properties to apicurio. properties?

@carlesarnal
Copy link
Member Author

Don't we also want to rename all registry. properties to apicurio. properties?

Not sure for all. We had a few ones that were clearly generic (auth, I'm looking at you) but then we have a pretty big set of properties that are very specific to Registry like registry.rest.artifact.download.maxSize. I don't have a strong opinion here, so I'll let you decide.

@carlesarnal carlesarnal force-pushed the standardize-configuration-properties branch from 1d44363 to 4e1f7a9 Compare April 10, 2024 12:13
@EricWittmann
Copy link
Member

I think we should change everything to be apicurio. across our all projects, but let's see what @jsenko thinks.

@jsenko
Copy link
Member

jsenko commented Apr 10, 2024 •
edited
Loading

I think we should change everything to be apicurio. across our all projects, but let's see what @jsenko thinks.

I'm in favor, as long as we try to keep the names consistent between Registry and Studio, of course. As an example, having apicurio.storage.type in Registry and apicurio.db.kind in Studio would be worse 😄

A big advantage I see would be easier sharing of documentation between projects, and code in common app components.

@EricWittmann
Copy link
Member

EricWittmann commented Apr 10, 2024 •
edited
Loading

OK that's two votes to change everything to apicurio. and one abstention. :) The aye's have it!

@EricWittmann
Copy link
Member

Also I agree - once the renames are all done and checked in for Registry 3, we can compare the table of properties and see where we have mismatches.

@carlesarnal carlesarnal force-pushed the standardize-configuration-properties branch 2 times, most recently from b4b205b to 3b577a8 Compare April 12, 2024 06:23
@carlesarnal
Copy link
Member Author

OK that's too votes to change everything to apicurio. and one abstention. :) The aye's have it!

Rename done, this is ready to go.

@carlesarnal carlesarnal linked an issue Apr 24, 2024 that may be closed by this pull request
Standardize configuration property names Apicurio/apicurio-epics#61
Closed
@carlesarnal carlesarnal force-pushed the standardize-configuration-properties branch from d86f6d4 to 26f4316 Compare April 24, 2024 08:38
jsenko
jsenko requested changes Apr 24, 2024
View reviewed changes
Copy link
Member

@jsenko jsenko left a comment

Choose a reason for hiding this comment

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

Added some comments, mostly consistency-related which are up for a discussion, but there are also some typos.

README.md Outdated
@@ -24,7 +24,7 @@ Which storage variant will be used is determined by the following configuration:

|Option|Command argument|Env. variable|
|---|---|---|
|Registry Storage Variant|`-Dregistry.storage.kind`|`REGISTRY_STORAGE_KIND`|
|Registry Storage Variant|`-Dapicurio.storage.kind`|`REGISTRY_STORAGE_KIND`|
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't REGISTRY_STORAGE_KIND be APICURIO_STORAGE_KIND instead? Same for the others.

Copy link
Member

Choose a reason for hiding this comment

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

This is still unresolved

@EricWittmann
Copy link
Member

When this is merged we need to remember to update the 3scale gitops repo. :)

jsenko
jsenko reviewed Apr 25, 2024
View reviewed changes
Copy link
Member

@jsenko jsenko left a comment

Choose a reason for hiding this comment

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

A few things, otherwise LGTM

README.md Outdated
@@ -24,7 +24,7 @@ Which storage variant will be used is determined by the following configuration:

|Option|Command argument|Env. variable|
|---|---|---|
|Registry Storage Variant|`-Dregistry.storage.kind`|`REGISTRY_STORAGE_KIND`|
|Registry Storage Variant|`-Dapicurio.storage.kind`|`REGISTRY_STORAGE_KIND`|
Copy link
Member

Choose a reason for hiding this comment

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

This is still unresolved

@carlesarnal carlesarnal merged commit 7b7296a into Apicurio:main Apr 29, 2024
20 checks passed
@carlesarnal carlesarnal mentioned this pull request Apr 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsenko jsenko jsenko approved these changes

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

Improve client credential server caching Standardize configuration property names
3 participants
@carlesarnal @EricWittmann @jsenko