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

Update: Provide the ability to view and set the default provider used for resource types. #1799

Merged
merged 19 commits into from
Oct 29, 2024

Conversation

panaaj
Copy link
Member

@panaaj panaaj commented Sep 23, 2024

Not all resource providers will support creating / updating resources e.g. charts-plugin.

When there are multiple providers registered for a resource type, the first plugin / provider registered will be used for write operations unless the provider to use is specified in the request.

Definition: default provider = provider used for write operations (when ?provider=<plugin-id> is not specified in the request).

Provide the ability to view and set the default provider for specific resource types and persist the selection bewteen restarts.

Implemented in this PR:

  1. GET /resources/{resourcetype}/_providers endpoint -> List the registered resource providers for the resource type
  2. GET /resources/{resourcetype}/_default endpoint -> Return the default provider for the resource type
  3. POST /resources/{resourcetype}/_default/{providerid} -> Set the default provider for a resource type

Example: Set the default provider for a routes to plugin with the id my-provider-id

POST "/resources/routes/_default/my-provider-id"

@panaaj panaaj self-assigned this Sep 23, 2024
@panaaj panaaj added the feature label Sep 23, 2024
@panaaj panaaj requested a review from tkurki October 2, 2024 05:21
@panaaj panaaj changed the title WIP: Update Chart Resource definitions and handling Update Chart Resource definitions and handling Oct 5, 2024
@tkurki
Copy link
Member

tkurki commented Oct 9, 2024

Making consistent and intuitive APIs is hard in the first place and when you have a single dimensioned url structure to do it with even more so....

This is not a strict opinion as this needs to change but just my view on how I'd structure the API:

Basically I am thinking about what should come first: resource type or provider? Now this PR makes it "provider first": /resources/providers/default/{resourcetype}. But does the API user ever address providers as a collection of all providers for all resource types - or is it always in connection with one resource type? I would have structured the api as

/resources/{resourcetype}/_providers/{providerId}/ with a special providerId value of default or to make it explicit _default. This way what you have after /resources/ is always the resource type.

I see also potential for a race condition - there is no guarantee that nothing else changed the default provider that you just set or that you think you set a moment ago.

Do you expect this api to be called by clients? r is this more like a server setting: "use this plugin as the target for write requests always" and this is more a question of plugin management and part of the server's admin / settings ui?

@tkurki
Copy link
Member

tkurki commented Oct 9, 2024

Is this actually two independent features?

  • ability to set default provider for write operations per resource type
  • charts api definition update

@panaaj
Copy link
Member Author

panaaj commented Oct 10, 2024

Is this actually two independent features?

  • ability to set default provider for write operations per resource type
  • charts api definition update

Technically yes. The thinking was to have the changes that support the use case together but will split them out.

@panaaj
Copy link
Member Author

panaaj commented Oct 10, 2024

Split definition and supporting provider changes out into #1808 and #1809

@panaaj panaaj changed the title Update Chart Resource definitions and handling Update: Provide the ability to view and set the default provider used for resource types. Oct 10, 2024
@panaaj
Copy link
Member Author

panaaj commented Oct 11, 2024

Do you expect this api to be called by clients?

I think it has value as both a server setting and a client callable end point (with suitable credentials supplied).

  • As a server setting you could provide both options to:

    1. Provider centric options e.g. Select a provider as the default for ALL of the resource types it supports
    2. Resource type centric options e.g. set the default provider for routes
  • For clients I see it as more of a resource type centric use case e.g. set the default provider for charts
    So I have no problem with the client endpoint structure being /resources/{resourcetype}/_providers/{providerId}

I see also potential for a race condition - there is no guarantee that nothing else changed the default provider that you just set or that you think you set a moment ago.

Are you talking about UI setting being made at the same time as the client API being used?
If so, given it is result of user action I don't feel it is any more likely than the same resource being updated at the same time which is also a possibility.

Where there is a race that is having to be managed is the loading and processing of the persisted settings against the available plugins.

When the API is initialising there is no guarantee that all plugins have finished being loaded by the plugin manager, so applying persisted settings is more challenging.

It would be nice if there were plugin manager lifecycle events to listen to in order to trigger tasks after the loading of plugins has completed at server start up.

src/api/resources/index.ts Outdated Show resolved Hide resolved
@panaaj
Copy link
Member Author

panaaj commented Oct 15, 2024

I will look to move the saved configuration to app.config.settings so it can be managed via the UI as well as the API.

Do I just create a new key at the root of the settings JSON resourcesApi or is the preference something like

{
   ...
   api: {
      resources: {...}
   }
}

@panaaj panaaj requested a review from tkurki October 24, 2024 03:23
Copy link
Member

@tkurki tkurki left a comment

Choose a reason for hiding this comment

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

Added 2 very minor changes, see the commit log.

Good to go, but this does not rebase on master cleanly - here is a squashed & rebased version, did not want to force push it on this branch, but feel free to do that & merge.

@panaaj panaaj merged commit 2de826f into master Oct 29, 2024
4 checks passed
@panaaj panaaj deleted the resoource-provider-charts branch October 29, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants