Preview options #12650

hmlnarik · 2022-06-22T15:18:15Z

hmlnarik
Jun 22, 2022

Problem statement

New features often take some time to stabilize. Features are often configurable from server config. One way to set server config in Quarkus distribution is via command line arguments. Sometimes it takes time to identify the right set of options.

Currently when there is an option introduced, it has to be maintained for backwards compatibility. Thus introducing a new option which has potential to be changed / removed in a future version often results in not introducing such an option at all. In order to set such an option for a tech-preview feature, one has to set a generic form --spi-spi-provider-option.

This makes it difficult to implement a set of feature options in Quarkus before the feature is fully complete and in supported mode.

Proposed solutions

I would like to propose introducing an optional preview option flag or support state (SUPPORTED, ~~TECH_PREVIEW~~ PREVIEW, potentially also DEPRECATED) on individual options. This way it would be possible to sketch options for tech-preview features in Quarkus even before the feature would be promoted to full support. A preview option has no guarantee on its existence in the following releases.
Such an option needs to be clearly marked as preview in the documentation and help page.
The alternative solution is to keep it as is. In other words, features would only be configurable in Quarkus via their generic form.

ahus1 · 2022-06-22T16:18:29Z

ahus1
Jun 22, 2022
Collaborator

Thanks for suggesting this. To explore option 1 a bit more:

everything that is not TECH_PREVIEW or DEPRECTED would be SUPPORTED as the default
How would TECH_PREVIEW and DEPRECATED behave differently from other configurations: Would TECH_PREVIEW / DEPRECATED "just" a piece of documentation in the --help description plus maybe log a warning when it is used?

Even with (1) being implemented, configuration via the Generic form should still be possible.

The benefit of (1) being that people would be more inclined to try out things when we have a simpler configuration plus a ready-to-go documentation. So as long as we keep it lightweight (printing a warning plus putting it into --help) I'd be +1 here.

1 reply

hmlnarik Jun 23, 2022
Author

I agree with that.

How would TECH_PREVIEW and DEPRECATED behave differently from other configurations: Would TECH_PREVIEW / DEPRECATED "just" a piece of documentation in the --help description plus maybe log a warning when it is used?

I'd say piece of documentation and [*] in the --help screen should suffice for now.

DGuhr · 2022-06-23T08:28:56Z

DGuhr
Jun 23, 2022

For information:
@stianst and me had a side discussion some time ago about this topic w.r.t. deprecation here

and I also roughly lined out my idea about deprecating options here: #12400 (comment)

Let me try to put this together with the widened scope for PREVIEW options:

DEPRECATED Configuration:
- Should be flagged as deprecated in our runtime-independant configuration wrapper.
- should not be shown anymore in the CLI help, but still work ("hidden")
- should be flagged in the documentation as deprecated (the new guides get the configuration objects from the aforementioned config wrapper objects). Where exactly: TBD. ( e.g. on all pages where they are used doesn't make sense, instead the docs build should have tests and comply when a DEPRECATED option is used outside the one place we want it to be used. Valid options I see: the all-config page seems a natural fit for me, perhaps with an extra switch to include/exclude them, alternatively or additionally a separate page).
- If a successor of the deprecated configuration option exists, should delegate the actual call to the successor.
- If no successor of the deprecated configuration exists, should still work with the conditions outlined in the other points.
- OPTIONAL: Should trigger a (warning?) log message when the server starts. This warning message has to be defined, but could e.g. include information about the targetted removal version, deprecated since, the reason, and the successor configuration option.
- Open Questions:
  - Should deprecated options be runtime specific? So "deprecated" server options are always deprecated also for the operator? I have an opinion, but to not bias let's discuss first :)
SUPPORTED options:
- should be the default, without further declaration. "As is" now.
PREVIEW options:
- Should show an indicator that they are in preview in the cli help and docs.
- OPTIONAL: Should show an (info?) log at server startup that they are used and likely to change or break in the future.

W.R.T. the guides integration there is another point to consider here: At the moment the guides are not tied to a Keycloak version, like e.g. quarkus does it with their dropdown here. In order to show what option got deprecated in what version and what option is supported in what version, we should think of a similar dropdown for our guides, but this leads to other implications (a guide used in v19 used a configuration option that was removed in v21, the configuration option still would need to be shown in the v19 guide, but not anymore in the v21 guide. Same applies for PREVIEW options that change throughout the preview phase or are supported in some version in the future)

When an option goes from SUPPORTED to DEPRECATED or from DEPRECATED into Nirvana, this surely has to be announced in the release notes, migration guide etc.

This is my (most surely incomplete) starting point so-far. I tried to leave the technical solution details out of it for now, to have a clearer look at the problem first. Feel free to extend/adapt/ask questions :)

3 replies

ahus1 Jun 23, 2022
Collaborator

Thank you for the extensive summary, I agree with everything except

[DEPRECTED] should not be shown anymore in the CLI help, but still work ("hidden")

I see a benefit in showing the deprecated options: The help should be complete and nothing should be hidden. Plus the description can contain information what should be used instead.

DGuhr Jun 23, 2022

I added it mainly because the CLI help is very big right now. Another solution to this problem could be to add a positional parameter, something around kc.sh start help include-deprecated to show them if needed (opt-in) or exclude-deprecated to remove them if not needed (opt-out), or use the existing help-all to include the deprecated ones.

We also had loose discussions around adding a search functionality to the cli help, so e.g. kc.sh help Database would only show the database help, but at the moment it's an all or nothing thing so to make it short I didn't want to overwhelm people with also including deprecated options per default. But I see your point. :)

hmlnarik Jun 23, 2022
Author

I'd prefer help-all which would also show deprecated options. It is similar to what e.g. GTK / Plasma commands do:

$ gimp --help
Usage:
  gimp [OPTION…] [FILE|URI...]

GNU Image Manipulation Program

Help Options:
  -h, --help                              Show help options
  --help-all                              Show all help options
[...]

hmlnarik · 2022-06-23T13:07:47Z

hmlnarik
Jun 23, 2022
Author

I can see we converge on PREVIEW, SUPPORTED, and DEPRECATED state and more discussion is needed for deprecation behaviour.
Therefore I suggest that we add a support state flag for each configuration option, currently with PREVIEW and SUPPORTED options. This state can be then easily extended with the DEPRECATED state once the discussion is concluded.
WDYT? @pedroigor @DGuhr @ahus1

2 replies

pedroigor Jun 23, 2022
Collaborator

Makes sense to me.

For now, I would also consider marking a whole option category (e.g.: storage, database, etc) to avoid the burden of setting (and error-prone) the state individually to each option.

For the new store configuration, all options from the storage category will be marked as preview.

DGuhr Jul 5, 2022

Sorry for coming back late. Also makes sense for me, as the requirements really diverge ( ;) ) between DEPRECATED and PREVIEW.

I created #12917 - it's a draft for now for one reason:

We are currently mixing the feature states "experimental" and "preview".
E.g. the CHM test now checks for this log message:

keycloak/quarkus/tests/integration/src/test/java/org/keycloak/it/storage/map/ChmStorageDistTest.java

Line 51 in 63614b1

cliResult.assertMessage("Experimental feature enabled: map_storage");

and it is "experimental", but we are talking about "preview". Now I added both to the CLI,
Not sure which one to take, but I think we should be consistent with the wording here.

When we align to this, I am happy to create an according issue and put it out of draft state.

hmlnarik · 2022-06-23T13:08:53Z

hmlnarik
Jun 23, 2022
Author

I've just updated the summary and initial post to use PREVIEW instead of TECH_PREVIEW to prevent confusion with support level.

1 reply

DGuhr Jun 23, 2022

updated my answer also to match.

Preview options #12650

Uh oh!

Uh oh!

hmlnarik Jun 22, 2022

Problem statement

Proposed solutions

Replies: 4 comments · 7 replies

Uh oh!

Uh oh!

ahus1 Jun 22, 2022 Collaborator

Uh oh!

Uh oh!

hmlnarik Jun 23, 2022 Author

Uh oh!

Uh oh!

DGuhr Jun 23, 2022

Uh oh!

ahus1 Jun 23, 2022 Collaborator

Uh oh!

Uh oh!

DGuhr Jun 23, 2022

Uh oh!

hmlnarik Jun 23, 2022 Author

Uh oh!

hmlnarik Jun 23, 2022 Author

Uh oh!

Uh oh!

pedroigor Jun 23, 2022 Collaborator

Uh oh!

Uh oh!

DGuhr Jul 5, 2022

Uh oh!

hmlnarik Jun 23, 2022 Author

Uh oh!

DGuhr Jun 23, 2022

hmlnarik
Jun 22, 2022

Replies: 4 comments 7 replies

ahus1
Jun 22, 2022
Collaborator

hmlnarik Jun 23, 2022
Author

DGuhr
Jun 23, 2022

ahus1 Jun 23, 2022
Collaborator

hmlnarik Jun 23, 2022
Author

hmlnarik
Jun 23, 2022
Author

pedroigor Jun 23, 2022
Collaborator

hmlnarik
Jun 23, 2022
Author