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

4214 restarted process instance #4420

Merged
merged 15 commits into from
Sep 12, 2024

Conversation

punitdarira
Copy link
Contributor

@punitdarira punitdarira commented Jun 8, 2024

Related to #4214

@mboskamp
Copy link
Member

Hey @punitdarira, thanks for contributing! Feel free to mark this PR as "Ready for review" when you are done with it and want someone from the team to review the changes. 👍

@mboskamp mboskamp removed their assignment Jun 20, 2024
@psavidis
Copy link
Contributor

Hey @punitdarira,
How is the progress going with this Pull Request? Are you still interested to work on it? If not, no worries, you can always open a new one in the future.

@punitdarira
Copy link
Contributor Author

Hey @punitdarira, How is the progress going with this Pull Request? Are you still interested to work on it? If not, no worries, you can always open a new one in the future.

Hi @mboskamp, @psavidis,
This PR is slightly dependent on #4334 for tests
I though of creating common tests for this and above mentioned PR.
I tested this feature locally and it was working as expected.
Currently I am working on the dependent ticket.

@punitdarira punitdarira marked this pull request as ready for review June 27, 2024 01:03
@punitdarira
Copy link
Contributor Author

  • Need to add the add column sql in db/upgrade. I wasn't sure how the naming of the file works.

@punitdarira
Copy link
Contributor Author

Hi! @mboskamp @psavidis,
This PR is ready to review. I updated existing test cases itself.

@tasso94 tasso94 requested review from psavidis and removed request for mboskamp September 6, 2024 09:40
@psavidis
Copy link
Contributor

psavidis commented Sep 9, 2024

Hi! @mboskamp @psavidis, This PR is ready to review. I updated existing test cases itself.

Hello @punitdarira. I'd like to review your PR. In the meantime, would you like to fix those conflicts?

To increase the visibility here, please see below:

A. CockroachDB will be discontinued with 7.22 release and onwards.

Conflict Root-cause: The files activiti.cockroachdb.create.history.sql, activiti.cockroachdb.drop.history.sql do not exist anymore on master
Solution: Drop them in your fork and omit the addition of the column for CockroachDB
References: Remove CockroachDB Ticket

Copy link
Contributor

@psavidis psavidis left a comment

Choose a reason for hiding this comment

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

Hello @punitdarira,

I went through the PR and tested your changes.
Could you consider deleting the files for CockRoachDB? (see my comment)

Then i can proceed with testing it against our CI.

@punitdarira
Copy link
Contributor Author

Hello @punitdarira,

I went through the PR and tested your changes. Could you consider deleting the files for CockRoachDB? (see my comment)

Then i can proceed with testing it against our CI.

Hi @psavidis,
The conflicts have been resolved.

@psavidis psavidis added ci:all-as Runs the builds for all application servers. and removed ci:all-as Runs the builds for all application servers. labels Sep 10, 2024
@psavidis
Copy link
Contributor

psavidis commented Sep 10, 2024

Hello @punitdarira,
I went through the PR and tested your changes. Could you consider deleting the files for CockRoachDB? (see my comment)
Then i can proceed with testing it against our CI.

Hi @psavidis, The conflicts have been resolved.

Hi @punitdarira ,

I've used a draft PR to execute the code against the CI.
The db migration suite is failing.

Could you please add the new column and index to the migration scripts? You can see this commit which fixes the problem.

PS: You can use the following maven command to test your changes by running the db migration test suite:

mvn verify -Pinstance-migration,database,h2 -f qa/test-db-instance-migration/pom.xml

@psavidis
Copy link
Contributor

psavidis commented Sep 11, 2024

Hello @punitdarira,
I went through the PR and tested your changes. Could you consider deleting the files for CockRoachDB? (see my comment)
Then i can proceed with testing it against our CI.

Hi @psavidis, The conflicts have been resolved.

Hi @punitdarira ,

I've used a draft PR to execute the code against the CI. The db migration suite is failing.

Could you please add the new column and index to the migration scripts? You can see this commit which fixes the problem.

PS: You can use the following maven command to test your changes by running the db migration test suite:

mvn verify -Pinstance-migration,database,h2 -f qa/test-db-instance-migration/pom.xml

Hello @punitdarira ,

We are interested to include this work in the upcoming 7.22 release.
Therefore, if you have time to make the contribution yourself today that would be perfect.

If not, that's not an issue, i will cherry pick it tomorrow from the draft PR that triggered the CI and proceed with merging your fork.

Thanks,
Petros

@psavidis psavidis self-requested a review September 11, 2024 13:18
@punitdarira
Copy link
Contributor Author

@psavidis ,
Sorry for the delay. I've pushed the changes

@psavidis
Copy link
Contributor

Thank you @punitdarira,

I've added tests and currently waiting for the CI results.

When done, i'll replicate the commit to your fork and then i'll proceed with the merging.

Best,
Petros

@psavidis psavidis merged commit 0509c7b into camunda:master Sep 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-as Runs the builds for all application servers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants