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

Generate a CRD with {type: string, format: date-time} #5867

Closed
matteriben opened this issue Apr 6, 2024 · 19 comments · Fixed by #5917
Closed

Generate a CRD with {type: string, format: date-time} #5867

matteriben opened this issue Apr 6, 2024 · 19 comments · Fixed by #5917

Comments

@matteriben
Copy link
Contributor

Is your enhancement related to a problem? Please describe

I'd like to generate a CRD with a timestamp of type=string, and format=date-time.

I can generate code from a CRD with a string using date-time format, but when I generate a CRD from that code it generates something else entirely. For example:

Original CRD snippet:

                timestamp:
                  type: string
                  format: date-time

Java code snippet generated from above CRD snippet:

    @com.fasterxml.jackson.annotation.JsonProperty("timestamp")
    @com.fasterxml.jackson.annotation.JsonSetter(nulls = com.fasterxml.jackson.annotation.Nulls.SKIP)
    private java.time.ZonedDateTime timestamp;

    @com.fasterxml.jackson.annotation.JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ssVV")
    public java.time.ZonedDateTime getTimestamp() {
        return timestamp;
    }

    @com.fasterxml.jackson.annotation.JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss[XXX][VV]")
    public void setTimestamp(java.time.ZonedDateTime timestamp) {
        this.timestamp = timestamp;
    }

CRD snippet generated from above Java code snippet:

              timestamp:
                properties:
                  dateTime:
                    properties:
                      date:
                        properties:
                          year:
                            type: integer
                        type: object
                      time:
                        properties:
                          nano:
                            type: integer
                        type: object
                    type: object
                  offset:
                    properties:
                      id:
                        type: string
                      totalSeconds:
                        type: integer
                    type: object
                  zone:
                    type: object
                type: object
            type: object

Describe the solution you'd like

I'd like for CRD fields of {type=string, format=date-time} to roundtrip from CRD to Java and back to CRD.

Describe alternatives you've considered

No response

Additional context

No response

@baloo42
Copy link
Contributor

baloo42 commented Apr 7, 2024

I don't think that roundtripping is supported at the moment.

The CRDGenerator for example does not use Jackson's JsonFormat annotation at the moment and there is no other way available to configure the format property in the JSONSchema.

See #5859 to get an overview about the missing JSONSchema properties.

@andreaTP
Copy link
Member

andreaTP commented Apr 8, 2024

From a historical POV when we started the java-generator we wished to have a full-blown roundtrip with the crd-generator working.
We stopped aiming for it due to the amount of work and the slightly different nuances of the 2 generators.

@matteriben do you have a specific use-case?

In general, I would happily accept PRs improving on this aspect(even if not completely solving it).
The guidance although, makes it tricky, as we should NOT rely on 3rd party annotations (in this case Jackson) in the crd-generator.

@manusa
Copy link
Member

manusa commented Apr 10, 2024

I'd like for CRD fields of {type=string, format=date-time} to roundtrip from CRD to Java and back to CRD.

As discussed on #5870, we're curious to know if the roundtripping is something that you need or would use. And specially what's the use-case for that. Please, if you can, share details about this 🙏

@matteriben
Copy link
Contributor Author

I'd like for CRD fields of {type=string, format=date-time} to roundtrip from CRD to Java and back to CRD.

As discussed on #5870, we're curious to know if the roundtripping is something that you need or would use. And specially what's the use-case for that. Please, if you can, share details about this 🙏

We are attempting to use the java-generator for contract first development of an operator. The CRDs we author may reference some existing Java types (#5843) which may appear simply as type: object in our CRD. The generated CRD has the details of the existing Java types filled in and otherwise mostly matches the CRDs we authored.

For us this issue wasn't really critical since we were okay to just drop format: date-time in our CRDs and maintain consistency between our original and generated CRDs. I opened the issue mostly just to document what I was seeing, and check if I was doing something wrong.

I saw the discussion about SchemaSwap on a related issue (#5866), and that got an internal discussion going on our side and we have a PR (#5885) that seems to fix both this issue and the related one.

@andreaTP
Copy link
Member

We are attempting to use the java-generator for contract first development of an operator.

🎉 amazing! Is it open source?
That's the reason why the java-generator has been created!

The generated CRD

Here you lost me... why would you want to re-generate the CRDs if you are doing contract-first?

@matteriben
Copy link
Contributor Author

🎉 amazing! Is it open source?

It will be, but it isn't yet publicly available.

The generated CRD

Here you lost me... why would you want to re-generate the CRDs if you are doing contract-first?

Currently the most compelling use case for crd-generation seems to be quarkus dev mode. As far as I can tell quarkus.operator-sdk.crd.apply only works with generated CRDs.

@andreaTP
Copy link
Member

It will be, but it isn't yet publicly available.

Please let us know when you have updates!

Currently the most compelling use case for crd-generation seems to be quarkus dev mode.

In this case, the correct fix should be done in the Quarkus Operator SDK, cc. @metacosm
Opened this one: quarkiverse/quarkus-operator-sdk#867

@matteriben
Copy link
Contributor Author

@andreaTP

I split this into two PRs. The initial PR now only adds the Format annotation. I have another PR here that tracks the source type and adds the crd-generator annotation for it.

#5912

Even with support from quarkus to apply our CRDs without generating them from source, we would need to run the crd-generator on existing types, such as Affinity, and stitch together a final CRD. Generating the full CRD from the java-code, if possible, seems preferable.

@andreaTP
Copy link
Member

Even with support from quarkus to apply our CRDs without generating them from source, we would need to run the crd-generator on existing types, such as Affinity, and stitch together a final CRD. Generating the full CRD from the java-code, if possible, seems preferable.

@matteriben I need to settle expectations on this one, the design goals of the generators in this project are:

  • crd-generator code first approach, you write the code and the rest gets generated
  • java-generator contract first, you write the contract(the crd) and the code gets generated

If I understand correctly, what you are trying to do is a sort of "pipeline":

  • a manually written crd with "holes"
  • processed by the java-generator to generate Java classes
  • re-process the Java classes with the crd-generator to emit the final crd

supporting such workflow has never been a goal of this project.
From the "theoretical" standpoint you are missing a single source of truth.
From the practical standpoint, you are going through 2 subsequent lossy translations:

  • the crd-generator ignores some Java elements(e.g. methods) that do not have an impact on the crd
  • the java-generator ignores the attributes(e.g. examples) that do not impact the usability of the generated Java code

I will support any bug fix that will arise from this exercise as long as it aligns with the original design goals, and, at the same time, I encourage you to take a more radical position over code or contract, a couple of possible scenarios:

  • code first: generate the Java code as a one-off and maintain it manually
  • contract-first: generate the CRD as a one-off and maintain it manually

@matteriben
Copy link
Contributor Author

matteriben commented Apr 18, 2024

I have another PR here: #5917

@metacosm
Copy link
Collaborator

Here you lost me... why would you want to re-generate the CRDs if you are doing contract-first?

Currently the most compelling use case for crd-generation seems to be quarkus dev mode. As far as I can tell quarkus.operator-sdk.crd.apply only works with generated CRDs.

I'm kind of lost as well… why would you want to re-apply the CRD if you're doing contract-first? In a contract-first scenario, it seems to me that the Quarkus Operator SDK shouldn't touch the CRD at all, since it should be, by design, outside of its control… In this situation, CRD generation should be deactivated altogether using quarkus.operator-sdk.crd.generate=false, shouldn't it? Am I missing something?

@andreaTP
Copy link
Member

@metacosm at best of my understanding, this is the missing bit: quarkiverse/quarkus-operator-sdk#867 basically the Quarkus Operator SDK only applies the generated CRD in dev mode.
I think that introducing a configuration: quarkus.operator-sdk.crd.external=resources/kubernetes/my-crd.yaml should help to account for this situation.
wdyt?

@metacosm
Copy link
Collaborator

@metacosm at best of my understanding, this is the missing bit: quarkiverse/quarkus-operator-sdk#867 basically the Quarkus Operator SDK only applies the generated CRD in dev mode. I think that introducing a configuration: quarkus.operator-sdk.crd.external=resources/kubernetes/my-crd.yaml should help to account for this situation. wdyt?

I'm not sure that this should be handled by QOSDK, to be honest. Presumably, the CRD is already defined and already applied to the cluster in a contract-first scenario. In my opinion, applying the CRD shouldn't be done automatically but only when the user is satisfied with the state the CRD is in. This isn't something that can be asserted by QOSDK.

@andreaTP
Copy link
Member

In my opinion, applying the CRD shouldn't be done automatically but only when the user is satisfied with the state the CRD is in. This isn't something that can be asserted by QOSDK.

I don't disagree, although this functionality is provided, as of today, for quarkus:dev mode + crd-generator and I can understand why a user would ask for it ...

@metacosm
Copy link
Collaborator

The functionality is provided, though, to automatically apply the updated CRD when the Java code changes (and only in that case), which we can detect automatically with Quarkus.
In the contract-first case, this wouldn't be as useful because we would need to set up monitoring of the external CRDs, which seems to me like potentially opening a can of worms (as we would need to have a cross-platform solution, be able to batch changes, so that the CRD doesn't get re-applied each time a letter is changed, etc.)

@andreaTP
Copy link
Member

when the Java code changes

The Maven Plugin re-generate the source code, so, watching those files is basically the same at the best of my understanding, or am I missing something?

@metacosm
Copy link
Collaborator

when the Java code changes

The Maven Plugin re-generate the source code, so, watching those files is basically the same at the best of my understanding, or am I missing something?

The CRD is not generated via Maven in QOSDK's case, it's done via the API whenever Quarkus triggers a hot reload (and the extension determines that whatever triggered the reload affects the CRD) during dev mode. The CRD is then applied only if it has been regenerated by the extension but this only happens whenever Quarkus is running (the CRD is not applied during a build, for example).

I guess we could have a marker file to indicate whether or not the CRD needs to be re-applied after Java code got regenerated whenever Quarkus starts the next time so maybe we wouldn't need to watch the CRD for changes. I haven't really used the Java generator so I'm not too familiar with how it works. Now that I think of it, maybe it could be possible to have Quarkus monitor the CRD and trigger Java code generation in dev mode. Not sure how feasible that would be or if it's even something we want to support, though.

@manusa
Copy link
Member

manusa commented Apr 23, 2024

Just like @metacosm indicates the CRD is the externally provided contract, it's not (or shouldn't) change under any circumstance.
It's (or is supposed to be) owned by an external entity.

Doesn't Quarkus provide a feature to deploy/apply any YAML file present in a given directory (e.g. JKube has something like that).

@andreaTP
Copy link
Member

Thanks everyone for the context.

I get the motivations, let's wait for @matteriben feedback as well.

Doesn't Quarkus provide a feature to deploy/apply any YAML file present in a given directory (e.g. JKube has something like that).

This can probably help too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants