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

Document webhook logic reuse pattern #47

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

gibizer
Copy link
Contributor

@gibizer gibizer commented Jun 27, 2023

No description provided.

@gibizer gibizer requested a review from abays June 27, 2023 09:55
Copy link

@jpodivin jpodivin left a comment

Choose a reason for hiding this comment

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

Overall I think this is a good pattern. But there are some stylistic issues that should be resolved.

webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
Comment on lines +97 to +108
3. Validation has multiple types, validation during create, update, or even
delete. The update validation has access to both the old and the new struct
value to be able to detect the change.

Choose a reason for hiding this comment

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

I would focus on this a bit more. Specifically, it would be a good idea to elaborate on requirements, that is MUST and MUST NOT, of each validation type.

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 not sure what kind of requirements you are after. Do you have an example?

Choose a reason for hiding this comment

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

Sure. I was thinking along the lines of:

Create Validation MUST return error of the NewInvalid type, if it encounters any invalid section of the created data structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified it in the doc

webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
@jpodivin
Copy link

Also, linking to an existing implementation of the pattern could be useful.

webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
webhooks.md Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
webhooks.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@gibizer gibizer left a comment

Choose a reason for hiding this comment

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

Thank you for the feedback I was able to fix most of it

webhooks.md Show resolved Hide resolved
Comment on lines +97 to +108
3. Validation has multiple types, validation during create, update, or even
delete. The update validation has access to both the old and the new struct
value to be able to detect the change.
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 not sure what kind of requirements you are after. Do you have an example?

webhooks.md Outdated Show resolved Hide resolved
Copy link
Contributor

@abays abays left a comment

Choose a reason for hiding this comment

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

/lgtm

webhooks.md Outdated Show resolved Hide resolved
webhooks.md Show resolved Hide resolved
webhooks.md Show resolved Hide resolved
@gibizer gibizer force-pushed the webhook-pattern branch 2 times, most recently from d31737a to ec3a794 Compare June 27, 2023 11:26
@gibizer
Copy link
Contributor Author

gibizer commented Jun 27, 2023

Also, linking to an existing implementation of the pattern could be useful.

Done.

Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

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

+1 from me

webhooks.md Outdated Show resolved Hide resolved
Copy link

@hjensas hjensas left a comment

Choose a reason for hiding this comment

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

LGTM.

gibizer added a commit to gibizer/ironic-operator that referenced this pull request Jun 30, 2023
gibizer added a commit to gibizer/nova-operator that referenced this pull request Jun 30, 2023
gibizer added a commit to gibizer/openstack-operator that referenced this pull request Jun 30, 2023
* split up the validation calls towards the service CRDs to have a
  separate ValidateCreate and ValidateUpdate call
* make sure that the service operators ValidateUpdate call gets the old
  version of the Spec struct so that they can compare

The pattern is documented in openstack-k8s-operators/dev-docs#47
openshift-merge-robot pushed a commit to openstack-k8s-operators/nova-operator that referenced this pull request Jul 5, 2023
gibizer added a commit to gibizer/openstack-operator that referenced this pull request Jul 5, 2023
* split up the validation calls towards the service CRDs to have a
  separate ValidateCreate and ValidateUpdate call
* make sure that the service operators ValidateUpdate call gets the old
  version of the Spec struct so that they can compare

The pattern is documented in openstack-k8s-operators/dev-docs#47
gibizer added a commit to gibizer/openstack-operator that referenced this pull request Jul 5, 2023
* split up the validation calls towards the service CRDs to have a
  separate ValidateCreate and ValidateUpdate call
* make sure that the service operators ValidateUpdate call gets the old
  version of the Spec struct so that they can compare

The pattern is documented in openstack-k8s-operators/dev-docs#47
@gibizer
Copy link
Contributor Author

gibizer commented Jul 7, 2023

The openstack-, ironic-, and nova-operator now aligned to this pattern. See openstack-k8s-operators/openstack-operator#383 for details. So I think we can merge this doc now

@abays abays merged commit e2b402c into openstack-k8s-operators:main Jul 7, 2023
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.

None yet

6 participants