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

Migrate to apiVersion v1 from v1aplha1 #1566

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

apupier
Copy link
Contributor

@apupier apupier commented Aug 10, 2023

fixes #1562

  • CRDs move apiVersion from camel.apache.org/v1alpha1 to camel.apache.org/v1
  • Migrate from KameletBinding to Pipe
    • Rename kind in CRDs
    • Renamed KameletBinding examples by changing suffix -binding to -pipe
    • Renamed folder and file names having binding
  • Updated doc
  • Updated Yaks tests

@apupier apupier force-pushed the 1562-UsePipeInsteadOfKameletBinding branch 4 times, most recently from 80305d4 to eef02f0 Compare August 14, 2023 07:29
Copy link
Contributor

@oscerd oscerd 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, but validator needs to be fixed.

@apupier
Copy link
Contributor Author

apupier commented Aug 22, 2023

Looks good, but validator needs to be fixed.

right. We need the camel k crds to be fixed first so that it exports types correctly. I just reported apache/camel-k#4683

@apupier
Copy link
Contributor Author

apupier commented Aug 22, 2023

also I think #1579 shoudl be fixed first and then rebase this PR to ensure there are no regressions

@claudio4j claudio4j marked this pull request as draft August 22, 2023 16:27
@claudio4j
Copy link
Contributor

Set as draft while we wait for the fix in yaks and the camel-kamelet api

@oscerd oscerd marked this pull request as ready for review August 23, 2023 09:35
@oscerd oscerd marked this pull request as draft August 23, 2023 09:35
@oscerd
Copy link
Contributor

oscerd commented Aug 23, 2023

#1579 is fixed.

@christophd
Copy link
Contributor

while fixing #1579 I realized that Camel JBang 4.0 is not able to handle v1 Pipe resources yet

@christophd
Copy link
Contributor

Right now YAKS tests use YAKS_CAMELK_KAMELET_API_VERSION=v1alpha1. If you set this to v1 (or remove the env setting as v1 is the default) the tests will create v1 Pipe resources instead of v1alpha1 KameletBinding

As mentioned earlier Camel JBang 4.0 runtime may not be able to handle Pipe resources though

@apupier
Copy link
Contributor Author

apupier commented Aug 23, 2023

Camel JBang 4.0 is not able to handle v1 Pipe resources yet

while fixing #1579 I realized that Camel JBang 4.0 is not able to handle v1 Pipe resources yet

There was a part of Camel Core/JBang which was able to use Pipe (if i remember well)
I noticed some parts that were missing, should come with 4.0.1 and 4.1.0 https://issues.apache.org/jira/browse/CAMEL-19732

@oscerd
Copy link
Contributor

oscerd commented Aug 23, 2023

If supporting Pipe is not completely working, we should wait to merge this one and release with just camel 4.0.0 release upgrade. Once everything is in place we could merge this one and release a 4.0.1

@apupier
Copy link
Contributor Author

apupier commented Aug 23, 2023

The migration of KameletBinding to Pipe sounds like an API break. Having it in a major version sounds better.

Is it a possibility of not releasing the Kamelets 4.0.0 and jump directly to 4.0.1 which will pick Camel Core/JBang 4.0.1 and Camel K 2.0.1?

@oscerd
Copy link
Contributor

oscerd commented Aug 23, 2023

I don't think it's good to not have a 4.0.0 release. But if the pipe upgrade is incomplete this PR cannot be merged

@apupier apupier force-pushed the 1562-UsePipeInsteadOfKameletBinding branch 2 times, most recently from 80cfead to f4a2aa7 Compare August 23, 2023 10:11
@oscerd
Copy link
Contributor

oscerd commented Aug 23, 2023

@davsclaus what are your thoughts on this?

@davsclaus
Copy link
Contributor

This would require Camel 4.0.1 to work with camel-jbang for the kamelet binding -> pipe renaming.

The v1alpha1 -> v1 works on 3.x and 4.x and is no problem.

@oscerd
Copy link
Contributor

oscerd commented Aug 23, 2023

So are you suggesting to release as is and merge this one only with 4.0.1?

@davsclaus
Copy link
Contributor

No its okay but just a known issue for camel-jbang with 4.0.0 that you need to use KameletBinding as the name. We will do 4.0.1 next month so it not very long time

@davsclaus
Copy link
Contributor

And Pipe will work on camel-k 2.0

@oscerd
Copy link
Contributor

oscerd commented Aug 23, 2023

Ok, I'll add this one before releasing, together with some new Kamelets

@apupier
Copy link
Contributor Author

apupier commented Aug 30, 2023

Rebased and migrated also newly added kamelet to be ready to integrate Camel K 2.0.1 release asap when it will be released (currently in vote)

@oscerd
Copy link
Contributor

oscerd commented Aug 31, 2023

Thanks

Copy link
Contributor

@christophd christophd left a comment

Choose a reason for hiding this comment

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

Had a look into the YAKS tests and added some comments why I think those are failing

.github/workflows/yaks-tests.yaml Outdated Show resolved Hide resolved
@@ -42,7 +42,7 @@ concurrency:

env:
YAKS_VERSION: 0.16.0
YAKS_RUN_OPTIONS: "--timeout=15m --local -e YAKS_CAMELK_MAX_ATTEMPTS=10 -e YAKS_JBANG_CAMEL_VERSION=4.0.0 -e YAKS_JBANG_KAMELETS_VERSION=4.0.0-SNAPSHOT -e YAKS_JBANG_KAMELETS_LOCAL_DIR=../../../kamelets -e YAKS_KAMELET_API_VERSION=v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question regarding -e YAKS_KAMELET_API_VERSION=v1alpha1

Is Camel 4.0.0 able to handle Pipe resources? Last time I checked this raised me some errors because Pipe has not been recognized by Camel JBang

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is supported completely from 4.0.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, 4.0.1 should handle it

I updated YAKS_JBANG_CAMEL_VERSION=4.0.0 to YAKS_JBANG_CAMEL_VERSION=4.0.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Camel 4.0.1 has not been released yet.
Will try with 4.0.1-SNAPSHOT but do not know how it will be able to pick it up.

@apupier
Copy link
Contributor Author

apupier commented Sep 5, 2023

it seems that Camel Jbang 4.0.0 is still used:
https://github.com/apache/camel-kamelets/actions/runs/6081335137/job/16496954462?pr=1566#step:7:173

 [jbang] Resolving dependencies...
[jbang]    org.citrusframework.yaks:yaks-parent:0.16.0@pom
[jbang]    org.citrusframework.yaks:yaks-jbang:0.16.0
[jbang]    org.citrusframework.yaks:yaks-runtime-core:0.16.0
[jbang] Dependencies resolved
[jbang] Building jar for YaksJBang.java...
0.16.0
[jbang] Resolving dependencies...
[jbang]    org.apache.camel:camel-bom:4.0.0@pom
[jbang]    org.apache.camel:camel-jbang-core:4.0.0
[jbang]    org.apache.camel.kamelets:camel-kamelets:4.0.0-RC1

which should explain that it is still failing:

   And Pipe aws-ddb-sink-pipe is available                                          # org.citrusframework.yaks.camelk.KameletSteps.pipeShouldBeAvailable(java.lang.String)
      com.consol.citrus.exceptions.TestCaseFailedException: Failed to retrieve pipe 'aws-ddb-sink-pipe' in state 'Running'

EDIT: Camel 4.0.1 has not been released yet

@oscerd
Copy link
Contributor

oscerd commented Sep 5, 2023

I don't know if it's really feasible to have yaks tests passing.

@oscerd
Copy link
Contributor

oscerd commented Sep 5, 2023

We need to take a decision, wait for 4.0.1 and introduce this stuff only at that point or merge without yaks tests, which it's not good anyway.

@oscerd
Copy link
Contributor

oscerd commented Sep 5, 2023

I don't know if it's really feasible to have yaks tests passing.

It's not YAKS fault.

@christophd
Copy link
Contributor

Can't we use Camel JBang 4.0.1-SNAPSHOT?

@oscerd
Copy link
Contributor

oscerd commented Sep 5, 2023

Can't we use Camel JBang 4.0.1-SNAPSHOT?

I don't think so, it doesn't pick it up.

Setting jbang 0.110.1 as default.
[jbang] Adding [https://github.com/citrusframework/yaks/] to /home/runner/.jbang/trusted-sources.json
[jbang] Adding [https://github.com/apache/camel/] to /home/runner/.jbang/trusted-sources.json
[jbang] Resolving dependencies...
[jbang]    org.citrusframework.yaks:yaks-parent:0.16.0@pom
[jbang]    org.citrusframework.yaks:yaks-jbang:0.16.0
[jbang]    org.citrusframework.yaks:yaks-runtime-core:0.16.0
[jbang] Dependencies resolved
[jbang] Building jar for YaksJBang.java...
0.16.0
[jbang] Resolving dependencies...
[jbang]    org.apache.camel:camel-bom:4.0.0@pom
[jbang]    org.apache.camel:camel-jbang-core:4.0.0
[jbang]    org.apache.camel.kamelets:camel-kamelets:4.0.0-RC1
[jbang] Dependencies resolved
[jbang] Building jar for CamelJBang.java...
4.0.0
Running YAKS tests for Kamelets
[jbang] Resolving dependencies...
[jbang]    org.citrusframework.yaks:yaks-parent:0.16.0@pom
[jbang]    org.citrusframework.yaks:yaks-jbang:0.16.0
[jbang]    org.citrusframework.yaks:yaks-runtime-core:0.16.0
[jbang]    org.apache.camel:camel-aws2-ddb:3.20.4
[jbang]    software.amazon.awssdk:dynamodb:2.20.17
[jbang]    org.apache.camel:camel-jackson:3.20.4
[jbang] Dependencies resolved

@christophd
Copy link
Contributor

Can't we use Camel JBang 4.0.1-SNAPSHOT?

I don't think so, it doesn't pick it up.

🤔 we would need to tell JBang to use this repository, too when resolving Maven artifacts. I think this must be added to the YAKS Camel JBang call. I can do this in YAKS.

Besides that I think we could also just keep using KameletBinding on the YAKS tests. Camel JBang is supposed to support both Pipe and KameletBinding and in the end we are keen to test the Kamelets here not the runtime

@oscerd
Copy link
Contributor

oscerd commented Sep 5, 2023

Can't we use Camel JBang 4.0.1-SNAPSHOT?

I don't think so, it doesn't pick it up.

🤔 we would need to tell JBang to use this repository, too when resolving Maven artifacts. I think this must be added to the YAKS Camel JBang call. I can do this in YAKS.

Besides that I think we could also just keep using KameletBinding on the YAKS tests. Camel JBang is supposed to support both Pipe and KameletBinding and in the end we are keen to test the Kamelets here not the runtime

Yes, this is another possibility and then switch after 4.0.1 @apupier could you do that?

@apupier
Copy link
Contributor Author

apupier commented Sep 5, 2023

Can't we use Camel JBang 4.0.1-SNAPSHOT?

I don't think so, it doesn't pick it up.

🤔 we would need to tell JBang to use this repository, too when resolving Maven artifacts. I think this must be added to the YAKS Camel JBang call. I can do this in YAKS.
Besides that I think we could also just keep using KameletBinding on the YAKS tests. Camel JBang is supposed to support both Pipe and KameletBinding and in the end we are keen to test the Kamelets here not the runtime

if i remember well, it is this repository which helped to catch that Camel JBang was not ready for Pipes so even if it was not be the initial purpose, it sounds like the current best way to test it.
if we do not remove KameletBinding in 4.0.0 of the Catalog, I fear it means to introduce quite an important change in 4.0.1

@apupier
Copy link
Contributor Author

apupier commented Sep 5, 2023

Yes, this is another possibility and then switch after 4.0.1 @apupier could you do that?

Looking to provide a PR with just the upgrade of Camel K CRDs which should be possible to be decorrelated from the kameletBinding -> Pipes move

EDIT: covered with this PR #1617

@apupier apupier force-pushed the 1562-UsePipeInsteadOfKameletBinding branch 5 times, most recently from f705a47 to c756273 Compare September 6, 2023 07:34
@apupier
Copy link
Contributor Author

apupier commented Sep 6, 2023

recreated branch on top of main (was too many conflicts to fixed, easier to redo).
Yaks test failing awaiting for Camel JBang 4.0.1

fixes apache#1562

* Renamed kind in CRDs
* Renamed KameletBinding examples by changing suffix -binding to -pipe
* Renamed folder and file names having binding
* Updated doc
* Updated Yaks tests

Signed-off-by: Aurélien Pupier <apupier@redhat.com>
@apupier apupier force-pushed the 1562-UsePipeInsteadOfKameletBinding branch from c756273 to f15f942 Compare September 25, 2023 13:25
@apupier
Copy link
Contributor Author

apupier commented Sep 25, 2023

rebased on main branch now that Camel 4.0.1 has been released but I think it will stil require apache/camel#11550

@oscerd
Copy link
Contributor

oscerd commented Sep 25, 2023

@christophd can you have a look?

Copy link
Contributor

@christophd christophd left a comment

Choose a reason for hiding this comment

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

Have had a look into the failing YAKS test. I think it should be fixed with adding "camel:cloudevents" dependency

test/aws-s3/aws-s3-to-http.yaml Show resolved Hide resolved
camel:cloudevents dependency is required because the pipe uses the CloudEvents data type transformers from Camel
@apupier apupier marked this pull request as ready for review September 26, 2023 06:37
@christophd
Copy link
Contributor

YES, tests are fixed!

@oscerd
Copy link
Contributor

oscerd commented Sep 26, 2023

Yuppie!

@oscerd
Copy link
Contributor

oscerd commented Sep 26, 2023

Let's merge, great work @apupier @christophd

@oscerd oscerd merged commit e788133 into apache:main Sep 26, 2023
5 checks passed
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.

Update apiVersion in kamelets to v1
5 participants