[#467] setup objecttypes through django-setup-configuration #489

SonnyBA · 2024-12-10T10:53:18Z

Fixes #467

Changes

Updates the existing django-setup-configuration steps and removed the DemoUserStep as that will be replaced by #482

…ion settings

This will be replaced by #485

SonnyBA · 2024-12-10T11:12:32Z

@danielmursa-dev I made some changes to the django-setup-configuration setup in this project which will also affect #485. To work on the earlier mentioned ticket you'll have to rebase onto this branch (or onto master if it gets merged).

stevenbal

Looks good generally, though I think the ObjectType configuration stuff is out of scope

stevenbal · 2024-12-10T11:59:34Z

src/objects/setup_configuration/steps/sites.py

+
+    def execute(self, model: SitesConfigurationModel) -> None:
+        for item in model.items:
+            site_kwargs = dict(domain=item.domain, name=item.name)


I implemented the same step when refactoring it in Open Notificaties: https://github.com/open-zaak/open-notificaties/pull/201/files#diff-9d74a1ba740219a222b8fb9dea30f135ebe2f5ad67f77ca87074ae55c13cb1ceR23

The behavior of the original step is a bit different, since it would let you specify an organization which would be used to create the name (Objects {organization}). To be honest I'm fine with either option (specifying organization or specifying name directly), but I think it would be good to pick one of these and be consistent.

Perhaps we could define this step in django-setup-configuration itself, so that we can reuse it?

@swrichards do you have an opinion on this?

I think it indeed makes sense to put this in django-setup-configuration itself. I am bit reluctant to delay these PRs though, so I would recommend leaving this in given that it works. It would be good if the two of you agree on whose step is leading to avoid divergence on the yaml structures, so the upgrade will be easier later on.

I guess the points to consider here (and in other projects?) are:

Upgrade the site related configuration and allow multiple sites to be configured (as is done in this PR)

Upgrade the site related configuration and allow configuration of one site (or update the current site, behavior of the previous site step for this project)

Remove the step altogether

Move the step to django-setup-configuration

Edit: made the list ordered to make referencing a bit easier

I am bit reluctant to delay these PRs

agree, though I think in that case it's important that the format is the same everywhere, otherwise we'll still end up with breaking changes. Let's use the step as it's implemented here

So we're gonna apply point 1 to all projects which use a site configuration step?

So we're gonna apply point 1 to all projects which use a site configuration step?
That sounds right to me.

In hindsight, it might actually be better to just allow one site to be configured, afaik we do not use multiple sites anywhere and if you now run on a freshly deployed instance, you end up with this:

Either that, or we should delete existing sites at the start of the configurationstep

Discussed this morning, this will be replaced by a configuration step from django-setup-configuration

src/objects/setup_configuration/steps/sites.py

src/objects/setup_configuration/tests/test_site_config.py

stevenbal · 2024-12-10T12:05:56Z

src/objects/tests/commands/test_setup_configuration.py

-    DEMO_PERSON="Demo",
-    DEMO_EMAIL="[email protected]",
-)
-class SetupConfigurationTests(TestCase):


Not sure if we want a similar test for the new setup. It might feel a bit excessive but I guess it does verify that all expected steps are executed and work together when used with an example.yaml file

@swrichards what do you think about this?

stevenbal · 2024-12-10T12:39:49Z

src/objects/setup_configuration/models/objecttypes.py

+from objects.core.models import ObjectType
+
+
+class ObjectTypeConfigurationModel(ConfigurationModel):


@swrichards @joeribekker I don't think this was very clear from the issue, but I think the scope of setup-configuration here was to be able to configure the connection from Objects API to Objecttypes API, and not to be able to configure objecttypes, right?

I think the question here is whether an objects-api/objecttypes-api can meaningfully function without being seeded with some object types of which objects-api has knowledge. I can imagine the bar of "minimum viable connectivity" here includes a certain level of seeding core objects, but I don't know enough about the Objects API to say for sure.

Objecttypes can be created with the API once a token is obtained. So, no, objecttypes should not be created via the YAML file.

Changed this behavior in 4a256d9. I'm wondering though if we should use a fixed identifier (for example objecttypes-api) and remove the ability to modify it.

src/objects/setup_configuration/steps/objecttypes.py

swrichards · 2024-12-10T14:54:06Z

src/objects/setup_configuration/steps/sites.py

+
+    def execute(self, model: SitesConfigurationModel) -> None:
+        for item in model.items:
+            site_kwargs = dict(domain=item.domain, name=item.name)


I think it indeed makes sense to put this in django-setup-configuration itself. I am bit reluctant to delay these PRs though, so I would recommend leaving this in given that it works. It would be good if the two of you agree on whose step is leading to avoid divergence on the yaml structures, so the upgrade will be easier later on.

src/objects/setup_configuration/steps/sites.py

swrichards · 2024-12-10T15:03:01Z

docs/installation/config_cli.rst

+Objecttypes require a corresponding ``Service`` to work correctly. Creating
+these ``Service``'s can be done by defining these in the same yaml file. ``Service``
+instances will be created before the ``ObjectType``'s are created.


Suggested change

Objecttypes require a corresponding ``Service`` to work correctly. Creating

these ``Service``'s can be done by defining these in the same yaml file. ``Service``

instances will be created before the ``ObjectType``'s are created.

Objecttypes require a corresponding ``Service`` to work correctly. Creating

these ``Service``'s can be done by defining these in the same yaml file (i.e. in the ``zgw_consumers`` block above). ``Service``

instances will be created before the ``ObjectType``'s are created.

See thread, this description is not relevant anymore.

swrichards · 2024-12-10T15:03:15Z

docs/installation/config_cli.rst

+   zgw_consumers:
+   services:


Suggested change

zgw_consumers:

services:

zgw_consumers:

services:

See #489 (comment)

swrichards · 2024-12-10T15:06:36Z

docs/installation/config_cli.rst

+         service_identifier: objecttypen-bar
+   ...
+
+.. note:: The ``uuid`` field will be used to lookup existing ``ObjectType``'s.


Suggested change

.. note:: The ``uuid`` field will be used to lookup existing ``ObjectType``'s.

.. note:: The `uuid` field is used to look up existing `ObjectType` entries through the `object-types` API of the service specified by `service_identifier`.

See #489 (comment)

src/objects/setup_configuration/models/sites.py

swrichards · 2024-12-10T15:09:42Z

src/objects/setup_configuration/models/objecttypes.py

+
+
+class ObjectTypesConfigurationModel(ConfigurationModel):
+    items: list[ObjectTypeConfigurationModel] = Field()


Suggested change

items: list[ObjectTypeConfigurationModel] = Field()

items: list[ObjectTypeConfigurationModel]

See #489 (comment)

swrichards · 2024-12-10T15:14:02Z

src/objects/setup_configuration/models/objecttypes.py

+from objects.core.models import ObjectType
+
+
+class ObjectTypeConfigurationModel(ConfigurationModel):


I think the question here is whether an objects-api/objecttypes-api can meaningfully function without being seeded with some object types of which objects-api has knowledge. I can imagine the bar of "minimum viable connectivity" here includes a certain level of seeding core objects, but I don't know enough about the Objects API to say for sure.

The step now lets you configure an connection with an objecttypes api

danielmursa-dev · 2024-12-11T11:05:35Z

src/objects/setup_configuration/steps/sites.py

+    namespace = "sites"
+    enable_setting = "sites_config_enable"


In this regard, I suggest having a unique naming convention for all projects and apps, so as not to create problems with different yaml files.
%s_%s' % (app_label, model_name)

something like how the url is made in admin

Suggested change

namespace = "sites"

enable_setting = "sites_config_enable"

namespace = "sites_site"

enable_setting = "sites_site_config_enable"

Good point, although the suggested namespace probably will clash whenever other projects allow their Site to be configured as well. Consider for instance the following configuration file:

# Objects-api sites_site_config_enable: true sites_site: items: .... # Objecttypes-api sites_site_config_enable: true sites_site: items: ....

I think prefixing it with the project name will fix this, for example objects_api_sites_config and objects_api_sites_config_enable.

@danielmursa-dev I think the collision of the namespaces, like mentioned above, also applies to the token configuration for certain projects?

yes exactly, everywhere the same namespaces must be used

@danielmursa-dev I think the collision of the namespaces, like mentioned above, also applies to the token configuration for certain projects?

Discussed through the chat, this comment can be ignored as the namespace collision will not happen (different configuration file per project). The project prefix is not necessary for Site configuration.

@SonnyBA OK , so let's continue with this format yes?

# %s_%s' % (app_label, model_name) sites_site_config_enable:` true sites_site: items: ....

I don't think it's necessary to change this in the steps in existing projects but this might be a good scheme for steps that are going to be added.

This reverts commit 433867d.

docs/installation/config_cli.rst

Co-authored-by: Sidney Richards <[email protected]>

swrichards · 2024-12-11T16:35:25Z

docker/setup_configuration/data.yaml

+objects_api_objecttypes_connection:
+  identifier: objecttypes-api
+  label: Objecttypen API
+  api_root: https://objecttypes.nl/api/v1/
+  api_connection_check_path: objecttypes
+  api_type: orc
+  auth_type: zgw
+  header_key: Authorization
+  header_value: Token foo
+  client_id: client
+  secret: secret
+  nlx: http://some-outway-adress.local:8080/
+  user_id: objects-api
+  user_representation: Objects API
+  timeout: 60


Discussed off-line: revert back to using zgw_consumers, this basically just replicates that logic. We found that there is no central config model that has a ForeignKey to a Service (as in other projects), rather each Object Type has such a foreign key. But because we said we wouldn't be configuring the actual object types here, we only have to configure Services (without having other models point to those services). TLDR: zgw_consumers is sufficient.

swrichards

See https://github.com/maykinmedia/objects-api/pull/489/files#r1880529232 -- will do a final review after that.

SonnyBA · 2024-12-12T10:52:28Z

The objecttypes connection part is now handled through the zgw-consumers configuration step. It might be practical to leave this PR open until there also is a configuration step provided from django-setup-configuration to setup site objects.

There should also be decided upon loading in objecttypes (locally) so that tokens also can be loaded in (as they will have no function without any objecttype attached to it).

SonnyBA · 2024-12-12T16:43:45Z

Closing this PR in favor of #492

SonnyBA added 11 commits December 6, 2024 12:22

[#467] update requirements

15b00a6

[#467] update objecttypes step

9380291

[#467] add tests

4c605ec

[#467] update documentation

38da55d

[#467] add missing optional requirement for zgw-consumers

19a8008

[#467] remove previous test suite

89f8d60

[#467] update docker compose setup & remove previous setup configurat…

8423892

…ion settings

[#467] update setup configuration project structure

6fb8d83

[#467] remove test override

09e092a

[#467] update django-setup-configuration site step

8f60c30

[#467] remove demo user documentation

f91b37d

This will be replaced by #485

SonnyBA self-assigned this Dec 10, 2024

SonnyBA added 2 commits December 10, 2024 11:56

[#467] apply code formatting

43964bd

[#467] include service configuration documentation

3461fca

SonnyBA marked this pull request as ready for review December 10, 2024 11:09

SonnyBA requested review from stevenbal, swrichards and danielmursa-dev December 10, 2024 11:10

stevenbal requested changes Dec 10, 2024

View reviewed changes

This was referenced Dec 10, 2024

Issue/django setup configuration tokens maykinmedia/objecttypes-api#135

Merged

♻️ [#200] Configure Autorisaties API connection via setup-configuration open-zaak/open-notificaties#201

Merged

swrichards requested changes Dec 10, 2024

View reviewed changes

SonnyBA added 4 commits December 11, 2024 11:32

[#467] Update objecttypes configuration step

4a256d9

The step now lets you configure an connection with an objecttypes api

[#467] update config namespace

fc99137

[#467] update sites config namespace

951e53f

[#467] remove Field call

9615bcf

danielmursa-dev reviewed Dec 11, 2024

View reviewed changes

SonnyBA added 2 commits December 11, 2024 12:11

[#467] update invalid connection test

045088e

[#467] Prefix sites configuration namespace with project name

433867d

Revert "[#467] Prefix sites configuration namespace with project name"

95bd575

This reverts commit 433867d.

SonnyBA requested review from swrichards and stevenbal December 11, 2024 11:39

swrichards reviewed Dec 11, 2024

View reviewed changes

docs/installation/config_cli.rst Outdated Show resolved Hide resolved

SonnyBA and others added 3 commits December 11, 2024 14:05

[#467] remove typo in documentation

7f4fd36

Co-authored-by: Sidney Richards <[email protected]>

[#467] update docker-compose example configuration

d4b1de1

[#467] remove unused configuration step

4f1767d

swrichards reviewed Dec 11, 2024

View reviewed changes

swrichards requested changes Dec 11, 2024

View reviewed changes

stevenbal mentioned this pull request Dec 12, 2024

Add support in ON for connectivity configuration via env vars open-zaak/open-notificaties#198

Closed

5 tasks

[#467] use zgw-consumers step to configure objecttypes connection

23f5756

SonnyBA mentioned this pull request Dec 12, 2024

[#483] add support for mozilla django OIDC db config #490

Merged

SonnyBA closed this Dec 12, 2024

		from objects.core.models import ObjectType


		class ObjectTypeConfigurationModel(ConfigurationModel):

	.. note:: The ``uuid`` field will be used to lookup existing ``ObjectType``'s.
	.. note:: The `uuid` field is used to look up existing `ObjectType` entries through the `object-types` API of the service specified by `service_identifier`.



		class ObjectTypesConfigurationModel(ConfigurationModel):
		items: list[ObjectTypeConfigurationModel] = Field()

[#467] setup objecttypes through django-setup-configuration #489

[#467] setup objecttypes through django-setup-configuration #489

Conversation

SonnyBA commented Dec 10, 2024

SonnyBA commented Dec 10, 2024

stevenbal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonnyBA Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenbal Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonnyBA Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swrichards left a comment

Choose a reason for hiding this comment

SonnyBA commented Dec 12, 2024

SonnyBA commented Dec 12, 2024

SonnyBA Dec 10, 2024 •

edited

Loading

stevenbal Dec 12, 2024 •

edited

Loading

SonnyBA Dec 11, 2024 •

edited

Loading