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

feat(crd-generator): Add support for WebhookConversion and NonConversion #5875

Closed
wants to merge 4 commits into from

Conversation

baloo42
Copy link
Contributor

@baloo42 baloo42 commented Apr 9, 2024

Description

Adds support for WebhookConversion and NonConversion to the CRDGenerator.

Refers to #5794

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

TODO

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 9, 2024

This approach introduces two new annotations @WebhookConversion and @NoneConversion. Both annotations will be detected on a custom resource class and decorate the CRD accordingly.
I call this approach "flattened" because I don't use a parent annotation and sub annotations e.g. for the service reference:

WebhookConversion

@Group("sample.fabric8.io")
@Version("v2")
@Kind("WebhookConversion")
@WebhookConversion(versions = { "v2", "v1" }, svcName = "conversion-webhook", svcNamespace = "my-namespace")
public class WebhookConversionExample extends CustomResource<WebhookConversionSpec, Void> {
}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: webhookconversions.sample.fabric8.io
spec:
  conversion:
    strategy: Webhook
    webhook:
      clientConfig:
        service:
          name: conversion-webhook
          namespace: my-namespace
      conversionReviewVersions:
      - v2
      - v1
  group: sample.fabric8.io
  names:
    kind: WebhookConversion
    plural: webhookconversions
    singular: webhookconversion
  scope: Cluster
[...]

NoneConversion

@Group("sample.fabric8.io")
@Version("v1")
@Kind("NoneConversion")
@NoneConversion
public class NoneConversionExample extends CustomResource<NonConversionSpec, Void> {
}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: noneconversions.sample.fabric8.io
spec:
  conversion:
    strategy: None
  group: sample.fabric8.io
  names:
    kind: NoneConversion
    plural: noneconversions
    singular: noneconversion
  scope: Cluster
[...]

Additional Notes

  • The annotations will be detected only on custom resource classes.
  • The configuration will be validated and the generator will fail if the annotation is used wrong. (e.g. if service reference and url are used at the same time)
  • The generator will fail, if the annotations are configured more than one time on the same custom resource kind. (e.g. both annotations or on multiple custom resource versions.

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 9, 2024

@andreaTP Can you (or someone else like @csviri) give me a first feedback on this? (any feedback is welcome..)

I will create in parallel another PR to prepare the test environment (Approval Tests and the test module improvements we talked about last week). After this is merged I will add to this PR additional tests.

TBD:

  • Is the flattened approach ok or do you prefer more annotations?
  • Are abbreviated variables ok or are long names preferred? (e.g. svcName vs serviceName)
  • Allow the annotation only on custom resource versions with storage=true?

@andreaTP
Copy link
Member

I think that @csviri is much more familiar than me with the subject, and I would appreciate his input on this one.
(I can do it, but is going to take more time)

Quick answers meanwhile:

Is the flattened approach ok or do you prefer more annotations?

I'm, personally, fine with it

Are abbreviated variables ok or are long names preferred?

I think that we are using long names across the codebase, I would keep the current style, e.g.:
https://github.com/search?q=repo%3Afabric8io%2Fkubernetes-client+serviceName&type=code

Copy link

sonarcloud bot commented Apr 11, 2024

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 23, 2024

Can probably be closed. See #5885 (comment)

@manusa
Copy link
Member

manusa commented Apr 30, 2024

Can probably be closed. See #5885 (comment)

Yes, maybe close it.
Once api-v2 is merged, we might want to add a test there to ensure the feature is working as expected.

@baloo42 baloo42 closed this Apr 30, 2024
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

3 participants