Move Kogito extension out of the Quarkus core repository #7815

mariofusco · 2020-03-12T13:30:22Z

The extension has been moved to the Kogito repository and is part of the Platform.

geoand · 2020-03-12T13:59:16Z

I assume that the docs need to be removed as well? And perhaps the references to kogito in some yaml metadata files?

geoand · 2020-03-12T14:17:09Z

Also, kogito needs to be removed from the CI configuration (which is why CI is failing)

mariofusco · 2020-03-12T14:56:01Z

@geoand I just removed kogito from ci-actions.yml (and this was also the reason why Misc3 was failing). I cannot find any other yaml referencing it, am I missing something? Also couldn't we leave kogito documentation there even if the extension is now hosted in our codebase?

gsmet · 2020-03-12T15:00:12Z

I think we should keep the doc in place (and adapt it if needed).

geoand · 2020-03-12T15:04:21Z

I believe we can leave the documentation but @maxandersen and @aloubyansky would have more to say about that case.

As for the YAML files, I found kogito mentioned in:
https://github.com/mariofusco/quarkus/blob/9d5f3455f92744815275ce1bfd3dc474a01280df/devtools/platform-descriptor-json/src/test/resources/fakeextensions.json#L1102

@maxandersen what does that file do?

evacchi · 2020-03-12T16:46:21Z

I don't think this failure is related, wdyt @geoand ?

geoand · 2020-03-12T18:56:12Z

Yeah, the JVM 13 failure is known (it seems that sometimes CI can't install JVM 13)

geoand · 2020-03-12T18:56:47Z

This is +1 from me. I'm just waiting for @maxandersen and @aloubyansky to provide feedback on my question above

evacchi · 2020-03-12T19:42:45Z

good let's just wait then, thanks!

maxandersen · 2020-03-12T20:17:56Z

Earlier today we talked about this move to be something for 1.4 ? are we saying master is now ready for 1.4 and 1.3.x is branched ?

@maxandersen what does that file do?

its used for testing the parsing of a platform - not important and can be kept as is.

maxandersen · 2020-03-12T20:19:10Z

btw. where is the new location ? and is that something that is releasable separate from kogito releases so we neither of us get stuck if we need to update the extension ?

aloubyansky · 2020-03-12T20:53:08Z

Master is 1.4.

evacchi · 2020-03-12T21:09:32Z

ouch we were hoping to include this in 1.3.0 :(

geoand · 2020-03-12T21:10:56Z

@gsmet ^

gsmet · 2020-03-12T22:37:03Z

No. It won’t be in 1.3. It’s too late.

gsmet · 2020-03-12T23:41:10Z

I was on my phone so here are more details:

you can't drop a PR like that the day before the Final release. We had 2 candidate releases for that.
I don't see any PR for the Platform? How will people install this extension? How will people migrate their existing applications?

As for the OptaPlanner fix, maybe it should be in a different PR so that we can include it in 1.3.1? That being said, I'm not sure it's the right fix because I'm worried people will have duplicate substitutions if they use both Kogito and OptaPlanner.

/cc @ge0ffrey

geoand · 2020-03-13T06:48:08Z

@gsmet the Optaplanner fix is only needed in the is PR because Kogito was bumped. It's not needed in the current master.

I agree that it is a little too late for 1.3

evacchi · 2020-03-13T08:07:37Z

unfortunately we had delays with releasing 0.8.0. We understand it's too late, we thought there was more time before Final. There is no PR for platform because we were waiting for merging this first; the branch should be here https://github.com/mariofusco/quarkus-platform/tree/k1251 /cc @mariofusco

it's fine to merge this to master. As for the proper fix to the OptaPlanner extension, we are already discussing with @ge0ffrey on the matter

geoand · 2020-03-13T08:15:20Z

So I am +1 for merging this to master now (the sooner the better to give the most time possible to prepare the rest of the stuff that needs to be done to have Kogito in the platform for 1.4)

ge0ffrey · 2020-03-13T09:35:19Z

...ptaplanner/runtime/src/main/java/io/quarkus/optaplanner/nativeimage/KogitoSubstitutions.java

+import com.oracle.svm.core.annotate.TargetClass;
+
+@TargetClass(className = "org.drools.core.rule.builder.dialect.asm.ClassGenerator")
+final class Target_org_drools_core_rule_builder_dialect_asm_ClassGenerator {


-1
This is a substitution that should happen in Kogito (= Drools), not in OptaPlanner. It's not OptaPlanner alone that will suffer from it. OptaPlanner should not work around it, Kogito should fix it. Kogito 0.8.0 is borked, we need a Kogito 0.8.1 release.

If we want to get the PR in, there is no other way. If you folks want to wait for a Kogito release that doesn't suffer from this, that's up to you :).
But if you want it in no, there is no other way

I am willing to comprise to merge it like this for Quarkus 1.3.0 as a temporarily workaround (*), if there's a commitment to revert it by Quarkus 1.3.1 by releasing and using Kogito 0.8.1. @mariofusco wdyt?

(*) Let's add a comment stating that it's a temporarily workaround.

In any case, this PR isn't making it into any 1.3.x line. We are talking purely for 1.4, whether it goes in as is, or with a new Kogito version

ge0ffrey · 2020-03-13T09:41:32Z

As for the OptaPlanner fix, maybe it should be in a different PR so that we can include it in 1.3.1? That being said, I'm not sure it's the right fix because I'm worried people will have duplicate substitutions if they use both Kogito and OptaPlanner.

/cc @ge0ffrey

@gsmet Master currently is fine, there is no bug there.
Just for the record: the "OptaPlanner fix" is not a bug in OptaPlanner, it's a regression in Kogito 0.8.0 from 0.7.1 that only surfaced in the OptaPlanner-Quarkus integration test.
This PR upgrade Kogito from 0.7.1 to 0.8.0 and contains a workaround, to get OptaPlanner to work, by Graal substituting something in Kogito-Drools. It does not fix the regression for other Kogito users, but they are less likely to run into it. I personally don't like this approach.
@mariofusco @evacchi I think the real fix is a hotfix Kogito 0.8.1 release asap that includes the substitution in Kogito-Drools itself. Wydt? Is that practical?

gsmet · 2020-03-13T10:42:14Z

Well, there's no rush. This won't be in either 1.3.0 or 1.3.1. It will be merged to master for 1.4, planned in about a month (so 3 weeks for the CR).

That being said, I would recommend to follow up on this soon to avoid having the same issue for 1.4.

evacchi · 2020-03-13T10:55:11Z

I think it's better if we merge this and then release the hotfix otherwise we can't still update the platform bom, which is the priority

geoand · 2020-03-13T11:30:51Z

I think it's better if we merge this and then release the hotfix otherwise we can't still update the platform bom, which is the priority

+1 from. @gsmet , @ge0ffrey ?

maxandersen · 2020-03-13T11:42:42Z

+1 on merging into master sooner rather than later.

gastaldi · 2020-03-13T17:59:56Z

+1 to merge to master. Any objections @gsmet ?

gastaldi · 2020-03-13T18:01:57Z

...ptaplanner/runtime/src/main/java/io/quarkus/optaplanner/nativeimage/KogitoSubstitutions.java

+import com.oracle.svm.core.annotate.TargetClass;
+
+@TargetClass(className = "org.drools.core.rule.builder.dialect.asm.ClassGenerator")
+final class Target_org_drools_core_rule_builder_dialect_asm_ClassGenerator {


Why is the class name different from KogitoSubstitutions?

It doesn't really matter since it's going away really really soon 😎

Well yeah, it's just that it's confusing to look at this class and see another name :)

The idea was that multiple substitutions might have been needed. For example like Netty.
It turned out that it wasn't needed 😊

mariofusco · 2020-03-16T08:24:50Z

@ge0ffrey if this is ok for you can you please approve it asap? Thanks.

geoand · 2020-03-16T08:27:12Z

If @ge0ffrey approves, I'll merge it right away so the Kogito team can proceed

gsmet · 2020-03-16T10:51:05Z

@ge0ffrey told he was ok with this here: #7815 (comment) so I would say let's proceed.

@mariofusco please follow up on the substitution though. We need a proper fix before 1.4 is released.

Thanks all.

mariofusco · 2020-03-16T16:05:10Z

@gsmet thanks. The proper fix is already on kogito master and will be available with kogito 0.8.1.

gsmet · 2020-04-14T20:13:57Z

@mariofusco did you change the GA of the extension? If so could you provide the new one as it requires a note in the migration guide.

Thanks!

mariofusco · 2020-04-15T07:55:23Z

@gsmet the new GA is org.kie.kogito:kogito-quarkus

Thank you.

mariofusco and others added 2 commits March 12, 2020 14:27

remove kogito extension from quarkus codebase

84abed8

Fix Optaplanner integration test

c74ab7d

mariofusco mentioned this pull request Mar 12, 2020

Remove Kogito extension from Quarkus codebase #7772

Closed

remove kogito from CI

c3625ad

ge0ffrey reviewed Mar 13, 2020

View reviewed changes

gastaldi approved these changes Mar 13, 2020

View reviewed changes

gastaldi added this to the 1.4.0 milestone Mar 13, 2020

gastaldi reviewed Mar 13, 2020

View reviewed changes

gsmet merged commit e5c3d91 into quarkusio:master Mar 16, 2020

gsmet changed the title ~~Remove Kogito extension from Quarkus codebase~~ Move Kogito extension out of the Quarkus core repository Apr 14, 2020

gsmet added the release/noteworthy-feature label Apr 14, 2020

Move Kogito extension out of the Quarkus core repository #7815

Move Kogito extension out of the Quarkus core repository #7815

Conversation

mariofusco commented Mar 12, 2020 • edited by gsmet Loading

geoand commented Mar 12, 2020

geoand commented Mar 12, 2020 • edited Loading

mariofusco commented Mar 12, 2020

gsmet commented Mar 12, 2020

geoand commented Mar 12, 2020

evacchi commented Mar 12, 2020

geoand commented Mar 12, 2020 • edited Loading

geoand commented Mar 12, 2020

evacchi commented Mar 12, 2020

maxandersen commented Mar 12, 2020

maxandersen commented Mar 12, 2020

aloubyansky commented Mar 12, 2020

evacchi commented Mar 12, 2020 • edited Loading

geoand commented Mar 12, 2020

gsmet commented Mar 12, 2020

gsmet commented Mar 12, 2020

geoand commented Mar 13, 2020 • edited Loading

evacchi commented Mar 13, 2020

geoand commented Mar 13, 2020

ge0ffrey Mar 13, 2020

Choose a reason for hiding this comment

geoand Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

ge0ffrey Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

geoand Mar 13, 2020

Choose a reason for hiding this comment

ge0ffrey commented Mar 13, 2020 • edited Loading

gsmet commented Mar 13, 2020

evacchi commented Mar 13, 2020

geoand commented Mar 13, 2020

maxandersen commented Mar 13, 2020

gastaldi commented Mar 13, 2020

gastaldi Mar 13, 2020

Choose a reason for hiding this comment

geoand Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

gastaldi Mar 13, 2020

Choose a reason for hiding this comment

geoand Mar 13, 2020

Choose a reason for hiding this comment

mariofusco commented Mar 16, 2020

geoand commented Mar 16, 2020

gsmet commented Mar 16, 2020

mariofusco commented Mar 16, 2020

gsmet commented Apr 14, 2020

mariofusco commented Apr 15, 2020

mariofusco commented Mar 12, 2020 •

edited by gsmet

Loading

geoand commented Mar 12, 2020 •

edited

Loading

geoand commented Mar 12, 2020 •

edited

Loading

evacchi commented Mar 12, 2020 •

edited

Loading

geoand commented Mar 13, 2020 •

edited

Loading

geoand Mar 13, 2020 •

edited

Loading

ge0ffrey Mar 13, 2020 •

edited

Loading

ge0ffrey commented Mar 13, 2020 •

edited

Loading

geoand Mar 13, 2020 •

edited

Loading