Removing oracle driver and associated tests #3381
Conversation
fjtirado
force-pushed
the
oracle_removal_proposal
branch
6 times, most recently
from
6350255 to
76a0197
Compare
|
@porcelli but these references are not linked to the actual driver jar. We can't even mention oracle? |
|
no problem to mention Oracle; I just find it misleading |
|
@porcelli Which else do you propose to remove?. Can you please point the place for the scripts? I was not aware of its existance, I just change the minimal to get rid of the driver (there are not specific test for Oracle in runtimes). |
|
@porcelli About the enum (which is, as expected, not used anywhere), it might be useful to keep it for people that actually use Oracle with out stuff. Same rationale probably apply to the scripts, but I want to take a look to them first. In any case thats a matter for a different PR, this one objective was to remove the driver without generating dead code behind. And I think that future PR should remove the whole DatabasetType enum, which does not really make sense. The existing Postgres check in the the producer, to be honest, it is pretty lame, but I do not want to mix PRs. |
|
@porcelli I think I found the script you were referring to https://github.com/apache/incubator-kie-kogito-runtimes/blob/main/addons/common/persistence/jdbc/src/main/resources/db/oracle/V1.35.0__create_runtimes_Oracle.sql. It makes sense according to the existing jdbc addon design. A design that is questionable, but it it not really related with this PR and is not going to be fixed by removing the ORACLE constant from the enum. On the contrary, it will be even more misleading, but without the ORACLE constants, the whole thing looks even more strange. |
|
@porcelli I agree that we shouldn't left anything behind, but I would approve and merge this PR regardless, at least to resolve this emergency. We could address any other leftover with another follow up PR, but at least this wouldn't be blocker for the release anymore. |
|
@mariofusco if this was the only remaining blocker for release; I'd agree with you. But since we still have time due other issues being worked.. I think we could have an adicional cycle to get this properly cleaned up. |
|
@porcelli I do not understand why we need to remove the script, but I will remove it anyway |
fjtirado
force-pushed
the
oracle_removal_proposal
branch
from
7f440a6 to
0305cf1
Compare
|
@porcelli Script and enum removed. |
| ANSI("ansi", "process_instances"), | ||
| ORACLE("Oracle", "PROCESS_INSTANCES"), | ||
| POSTGRES("PostgreSQL", "process_instances"); |
There was a problem hiding this comment.
why instead of completely remove the enum, use something more generic... better aligned with the intention. Something like:
- JSONB_DATABASE
- BLOB_DATABASE
- ANSI_DATABASE
There was a problem hiding this comment.
@porcelli The enum was basically useless (as shown in the last commit, there are better ways to find out if we should use the correlation feature, which currently only works in postgres) ? Therefore, removing it is better than start a discussion (not related with the intent of the PR) over which names we should give to an enum that is not needed.
Another reason I cannot proceed with your request change is that those values you are proposing are implementation dependendant. The only feature (in runtimes, and this enum is for runtime) that is using jsonb is the serverless workflow correlation and that will soon change when we retake implementation of this feature (that was interrupted one year ago), so it does not make sense at all to expose this soon to be reverted implementation detail into a public enum.
|
thank you @fjtirado for the changes! |
fjtirado
force-pushed
the
oracle_removal_proposal
branch
from
0305cf1 to
e1793e2
Compare
fjtirado
force-pushed
the
oracle_removal_proposal
branch
from
e1793e2 to
3539a13
Compare
* Removing oracle driver and associated tests * Removing Oracle container * Removing oracle enum and script
Removing oracle jdbc driver from kogito_test_utils because of legal issues
Merge with apache/incubator-kie-kogito-apps#1971