Allow to configure internal wiki provider via environment variables

[NobodysNightmare]

Ticket

https://community.openproject.org/wp/XWI-44

[github-actions]

Warning

Flaky specs

• rspec ./modules/my_page/spec/features/my/work_package_table_spec.rb[1:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23903, linked for reference only):

- `rspec ./modules/my_page/spec/features/my/work_package_table_spec.rb[1:1:1]`

Treat this as a standalone task, unrelated to PR #23903. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23903 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @NobodysNightmare to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @NobodysNightmare, and request a review from @NobodysNightmare.
On every commit, set @NobodysNightmare as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

[Kharonus]

🔴 wait, does that mean, if we seeded an internal provider with env we cannot disable it anymore?

Edit: confirmed below. Was this a requirement I missed?

[Kharonus]

Edit 2: I get it now, this "seeded by ENV" was kind of a red hering for me. It is more "enabled/disabled by ENV".

🟡 The change in the controller is okay, but I ask myself, if we maybe shouldn't touch the controller and just amend the update contract?

[NobodysNightmare]

It's not using a contract at all 😅

But yeah, that would be the alternative. I guess a better name would be configured_by_env?

WDYT?

[Kharonus]

🟡 I'm confused by the format. Why a hash? The only thing we actually accept here is { enabled: true|false }. So, a single boolean would do the job just fine, right?

[NobodysNightmare]

This is mostly about forward-compatibility.

Essentially we didn't make enabling/disabling the internal provider a Setting so far, but we decided to introduce a Provider object that would carry the configuration. So it would be super confusing (IMO) if we had Setting.internal_provider_enabled?, but it wouldn't decide whether the internal provider was enabled, because we use InternalProvider.first.enabled? for that.

Thus I am configuring the entire internal provider through this hash. Like I configure entire external providers through a hash/array soon as well.

The thing is that right now enabled is the only thing that can be configured, but who knows what the future will hold.

[Kharonus]

❓ logic here is now: we seed (force write) the internal provider, if

a) it is not here
b) it is already here, but we have an setting by ENV

Is this usually part of the logic of the seeders? honest question, I have no plan. :D The option b) somehow feels like this shouldn't be handled by a seeder.

[Kharonus]

On the other hand I'm very open here for a sensible argument, why the seeder is exactly the correct place.

I just wanted to express, that when I would be searching for this logic, my first thought wouldn't be "seeder".

[NobodysNightmare]

When you look at the EnvData::OpenIDConnect::ProviderSeeder you will see that second-half statement there as well.

I think the one thing that's ucommon here, is that we seed something that may be configured through env, but even if it isn't we want to seed it anyways. E.g. the OIDC seeder seeds nothing without env or something with env.

This one here is supposed to either make one initial seeding round or (if a specific configuration is intended) ensure a specific configuration whenever seeding.

[NobodysNightmare]

tl; dr: I wouldn't know how to phrase the applicable? better/more logical. I am kinda open between having this a BasicData or an EnvData seeder.

My logic so far was that it's not always based on env data, but the app needs the seeded value to properly function, thus BasicData.