search

apache / camel-k-runtime

Apache Camel K runtime
https://camel.apache.org
Apache License 2.0
62 stars 52 forks source link

Ref #953: Update DSLs metadata #954

Closed essobedo closed 1 year ago

essobedo commented 1 year ago

fixes #953

Motivation

In Camel-Quarkus 2.16.0 several DSLs have been improved to support the native mode, the metadata needs to be updated consequently.

Modifications:

Release Note

Update DSLs metadata
essobedo commented 1 year ago

I'm not sure if it is a good idea but I would like to use the DSLs metadata as build hints for camel-k. If it is acceptable for you, supporting those hints in camel-k would be my next PR once this PR is merged and camel-k-runtime released

oscerd commented 1 year ago

@squakez

oscerd commented 1 year ago

@essobedo can you please rebase?

essobedo commented 1 year ago

@essobedo can you please rebase?

Done

essobedo commented 1 year ago

The idea is good, but we need to make sure to have a correspondent development on Camel K side. IMO, it would be better that you prepare a different PR for each language that you want to add. Locally, on Camel K side, you can use the Camel K runtime project with your local changes (make images [CAMEL_K_RUNTIME_DIR=/path/to/camel-k-runtime-project]) to test the change you're doing on Camel K side as well. You may start with Java support that is a top one required apache/camel-k#3023 (I see you've self assigned, great!). Once the development is ready and tested both on Camel K and runtime sides, you can issue a PR on both side. We'd merge it on the runtime, and immediately we can test in Camel K side (it will take the related snapshot of the runtime).

I'd like to avoid to merge this as we would say we support something that is not yet ready on Camel K side. Wdyt?

I understand your point of view but it is an egg and chicken problem, without this PR a dynamic logic cannot be implemented in Camel-K. Moreover, unless I missed something, according to what I can see in the code, the metadata are just ignored so adding new ones has no effect in Camel-K which also means that the current metadata native is not supported either.

Anyway, let me do what you propose.

essobedo commented 1 year ago

Done

squakez commented 1 year ago

Yeah, that's the reason I proposed you to prepare both Camel K runtime and Camel K developments locally before pushing the related PRs. When I need something that affects both projects, the approach I use is the following one:

In this way we minimize the risk to do some development on the runtime that is not yet ready in Camel K, just doing one thing at a time. Let me know if I can help to make this approach working for you as well.

essobedo commented 1 year ago

@squakez is it good enough for you now? Can we merge it?

squakez commented 1 year ago

@squakez is it good enough for you now? Can we merge it?

I'd prefer not, but let me elaborate the reason why :)

We're about to release Camel K 1.12 and with it the related runtime (based on main). If we have unreliable metadata in the catalog, we may screw up Camel K or any other tooling that is using the catalog (there are tools based on Camel K that makes use of the catalog).

The approach I was proposing hence is to reduce the uncertainty. I'd be happy to have PR both on Camel K and Camel K runtime before being able to merge on both side. That's the reason why I was proposing to make a work local to ensure you have both bits prepared and later issue the PRs so we can review and validate on both sides.

Once they are both reviewed and accepted, the Camel K Runtime would be merged as first, so that we publish the related snapshot dependency. Once that is done we'd re-run Camel K PR checks and make sure the feature is really complete.

I'd suggest to switch each DSL separately along you do the related development in Camel K for that reason. I know it's a bit cumbersome tuning the local development environment, but it's something we use to do frequently when there are changes on both side of the coin. Feel free to reach out if you need any support with that.

essobedo commented 1 year ago

OK, I see thx for the clarifications, I did not know that you release a Camel-K immediately after releasing a Camel-K runtime, I thought that I would have some time in between. TBH I'm not yet ready to propose a PR in Camel K right now, I only have a POC so far, that I need to clean up and improve to have it "production ready".

How long do I have to propose both PRs before you release both Camel-K and Camel-K runtime?

squakez commented 1 year ago

No rush, as soon as it is ready, you can issue both PRs. Then, the release cadence will be likely every couple of months or something like that, we don't have a very strict rule. There is no need to have a final release of Camel K Runtime to test it in Camel K as we are using the latest snapshot in the Camel K main branch. So, once both PRs are available, we'll merge Camel K runtime PR first (which will publish the change in the snapshot dependency) and immediately we can re-trigger the checks on Camel K PR (which will pick it up those changes just merged in Camel K runtime - see https://github.com/apache/camel-k/blob/a51b51da80331c9a3cbbf31f88f7959bbfc83ecf/script/get_catalog.sh#L38).

essobedo commented 1 year ago

@squakez ok I start working on the PR for Camel-K, to propose it asap

essobedo commented 1 year ago

Maybe we could add some unit test to validate those new fields but for the rest lgtm.

For me, a unit test is meant to validate some logic/behavior, here there is no logic, only pure metadata, so what kind of test do you expect exactly?

squakez commented 1 year ago

@essobedo something like this could be an idea https://github.com/apache/camel-k-runtime/pull/943 but it's a suggestion, you consider if it makes sense or not.

essobedo commented 1 year ago

@essobedo something like this could be an idea #943 but it's a suggestion, you consider if it makes sense or not.

I see, no problem, I can add it

essobedo commented 1 year ago

It is added

essobedo commented 1 year ago

Is it good enough to be merged? I would need a snapshot with these changes to be able to re-add this E2E test https://github.com/apache/camel-k/pull/4021/files#diff-4a3f793078defcff739c742870ba182fc1b17b41998b57bfa4673535420198c1R47-R62

squakez commented 1 year ago

Yes, please go ahead when you think the Camel K PR is completed and you need the snapshot from the runtime. Mind that, once merged, it will take about one hour for the snapshot to be available.