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

✨ [#100] Add ConfigurationStep for Service model #99

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Nov 11, 2024

Closes #100

  • Adds django-setup-configuration as an optional dependency via pip install zgw-consumers[setup-configuration]
  • Adds a ServiceConfigurationStep that can be reused in projects that use zgw-consumers

@stevenbal stevenbal changed the title Feature/django setup configuration ✨ Add ConfigurationStep for Service model Nov 12, 2024
@stevenbal stevenbal changed the title ✨ Add ConfigurationStep for Service model ✨ [#100] Add ConfigurationStep for Service model Nov 12, 2024
@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch 3 times, most recently from 7541613 to 62cb25c Compare November 12, 2024 14:17
@stevenbal stevenbal marked this pull request as draft November 12, 2024 14:17
@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch 4 times, most recently from 85b7850 to fb28f8a Compare November 12, 2024 14:30
@stevenbal stevenbal requested a review from swrichards November 12, 2024 14:44
@stevenbal
Copy link
Contributor Author

@swrichards I think this is ready for a first review, though we'll probably keep it in draft until it's functional in a PR in another project (Open Forms for example)?

Comment on lines +8 to +10
# TODO these should probably be defined in simple_certmanager and referred to?
# client_certificate: FilePath = DjangoModelRef(Service, "client_certificate")
# server_certificate: FilePath = DjangoModelRef(Service, "server_certificate")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm checking with Sjoerd if Dimpact uses these currently, if not this might be something for later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sjoerd told me that Dimpact doesn't use this currently, so I'm leaving this out of scope for now

@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch 2 times, most recently from 59b2602 to 5af30fd Compare November 15, 2024 10:22
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

I would like to see more tests with a minimal YAML entry and one with all possible properties configured so that it's obvious when the minimal format is extended in the future, as that is a future breaking change that devops needs to act on

@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch 3 times, most recently from 6bb68b2 to f6cdb10 Compare November 19, 2024 08:41
@stevenbal
Copy link
Contributor Author

@sergei-maertens added tests for this and also added client_certificate and service_certificate (commented out) to the config file with all fields

@stevenbal stevenbal marked this pull request as ready for review November 19, 2024 08:42
@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch 2 times, most recently from adef24a to f81f32d Compare November 19, 2024 11:30
@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch 4 times, most recently from e749ac4 to 79b917c Compare November 25, 2024 08:20
Copy link
Collaborator

@swrichards swrichards left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small things from my side.

zgw_consumers/contrib/setup_configuration/models.py Outdated Show resolved Hide resolved
zgw_consumers/contrib/setup_configuration/steps.py Outdated Show resolved Hide resolved
zgw_consumers/contrib/setup_configuration/steps.py Outdated Show resolved Hide resolved
zgw_consumers/contrib/setup_configuration/steps.py Outdated Show resolved Hide resolved
zgw_consumers/contrib/setup_configuration/steps.py Outdated Show resolved Hide resolved
zgw_consumers/contrib/setup_configuration/steps.py Outdated Show resolved Hide resolved
zgw_consumers/contrib/setup_configuration/steps.py Outdated Show resolved Hide resolved
zgw_consumers/contrib/setup_configuration/models.py Outdated Show resolved Hide resolved
@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch 3 times, most recently from bcfa9f2 to d260acb Compare November 29, 2024 09:36
@stevenbal stevenbal requested a review from swrichards November 29, 2024 09:40
Copy link
Collaborator

@swrichards swrichards left a comment

Choose a reason for hiding this comment

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

LGTM ✨

which can load any number of Services, configured in a YAML file
* use lowercase for root level keys in YAML files (e.g. `zgw_consumers_config_enable`)
* use `identifier` instead of `slug` in config files
* add `setup_config_` prefix to test files
* remove `is_configured` and `validate_result`
@stevenbal stevenbal force-pushed the feature/django-setup-configuration branch from d260acb to 2e4516a Compare December 2, 2024 08:52
@stevenbal stevenbal merged commit ecfa9b2 into main Dec 2, 2024
7 checks passed
@stevenbal stevenbal deleted the feature/django-setup-configuration branch December 2, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add django-setup-configuration as optional dependency to configure Services
3 participants