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

fix: Update citrus-remote to Citrus version 4 #6

Merged

Conversation

maletic
Copy link
Contributor

@maletic maletic commented Jul 15, 2024

This pull request updates citrus-remote to be compatible with Citrus version 4. The update addresses several dependency issues and ensures that our framework leverages the latest features and improvements of Citrus.
Key Changes:

  1. Updated the Citrus dependency to version 4
  2. Modified code and configurations to accommodate breaking changes and new features in Citrus version 4
  3. Ensured all tests pass with the new version

Testing:

The following repositories were used to test these changes:

  1. Citrus: https://github.com/maletic/citrus/tree/fix-test-classes-not-found-in-jar (contains a fix for citrus test class loading)
  2. Citrus-Remote: https://github.com/maletic/citrus-remote/tree/update-citrus-remote-to-newest
  3. Citrus-Playground: https://github.com/maletic/citrus-playground/tree/update-citrus-remote

This PR depends on: citrusframework/citrus#1188

@bbortt
Copy link
Contributor

bbortt commented Jul 15, 2024

This PR depends on: citrusframework/citrus#1188

has been merged, but requires a "fix" release @christophd

@bbortt
Copy link
Contributor

bbortt commented Jul 15, 2024

I love how the whole project contains 0 tests 🤣 not the best for a testautomation framework haha

@maletic I've looked it through and cleaned up the code- here's the pr: maletic#1.

I wonder if we could include https://github.com/maletic/citrus-playground/tree/update-citrus-remote into the pipeline here.

@christophd it would be cool if you could "do that thing with the copyright headers again" (you did it for citrusframework/citrus using intellij idea).

@bbortt
Copy link
Contributor

bbortt commented Aug 18, 2024

@maletic & @turing85 please see maletic#2. I've added the sample repository of @maletic to the build - if you're ok with that?

I am no maintainer, so @christophd will have to approve the workflow in this PR afterwards.

with the changes or additions mentioned above: /lgtm

@turing85
Copy link
Contributor

turing85 commented Aug 18, 2024

I am against adding the tests as-is. For one, the quarkus versions are outdated. For another, we could rewrite the tests purely in citrus to not pull-in another framework.

Overall, this is unrelated to the actual change. Just like @christophd 's idea to replace spark/jetty with vert.x, I see this as a separate task that should be handled separately.

@bbortt
Copy link
Contributor

bbortt commented Aug 18, 2024

Overall, this is unrelated to the actual change.

only partially, I think. I do understand that you want to have the change released, however...

  1. the module is not continuously tested as it is now
  2. I fear that once this PR is merged, nobody is gonna care about tests anymore
    • (or the move to vert.x for that matter)

note that I've additionally checked and updated all dependencies; there were few breaking changes involved too.

anyway, the changes are there, I've submitted the PR. because of that, I might accept the PR without the sample too. as already said, I've no administrative rights on citrus-remote, so @christophd will have to do the last call on this one.

pom.xml Outdated Show resolved Hide resolved
@maletic
Copy link
Contributor Author

maletic commented Aug 20, 2024

I increased the versions in the poms to the ones in the PR, so it ready for merging 😄

<groupId>com.consol.citrus</groupId>
<version>3.5.0-SNAPSHOT</version>
<groupId>org.citrusframework</groupId>
<version>4.3.0-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should set this to 4.3.1.-SNAPSHOT

Copy link
Contributor

@bbortt bbortt Aug 21, 2024

Choose a reason for hiding this comment

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

because you want to sync it with citrus? I think that's a bad idea. what if there's a bugfix in citrus-remote, but not in citrus? versions get out of sync so easily. I think it should just be 4.0.0 (and -SNAPSHOT).

Copy link
Contributor

Choose a reason for hiding this comment

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

@christophd I think you'd agree here (before merging)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should try to keep in sync and yes it may be tedious. I would try to only differ in minor releases when there is a bugfix. As a long-term objective we should bring citrus-remote back to citrus core project

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, if that's the goal :)

citrus-remote-server/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@turing85
Copy link
Contributor

<maven.version>3.8.4</maven.version>

- <maven.version>3.8.4</maven.version>
+ <maven.version>3.9.8</maven.version>

@turing85
Copy link
Contributor

https://github.com/citrusframework/citrus-remote/blob/38d346e40f625d8271099a33ea4af66c8334a263/pom.xml#L27C12-L27C18

- <maven.antrun.plugin.version>3.0.0</maven.antrun.plugin.version>
+ <maven.antrun.plugin.version>3.1.0</maven.antrun.plugin.version>

@turing85
Copy link
Contributor

<maven.clean.plugin.version>3.0.0</maven.clean.plugin.version>

- <maven.clean.plugin.version>3.0.0</maven.clean.plugin.version>
+ <maven.clean.plugin.version>3.3.1</maven.clean.plugin.version>

@turing85
Copy link
Contributor

<maven.dependency.plugin.version>3.0.2</maven.dependency.plugin.version>

- <maven.dependency.plugin.version>3.0.2</maven.dependency.plugin.version>
+ <maven.dependency.plugin.version>3.6.0</maven.dependency.plugin.version>

@turing85
Copy link
Contributor

I think we should sync the plugins versions before merging...

@turing85
Copy link
Contributor

@christophd so... we gonna merge?

Copy link
Member

@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.

Many thanks, looks good. Just the SNAPSHOT project version should be fixed

<groupId>com.consol.citrus</groupId>
<version>3.5.0-SNAPSHOT</version>
<groupId>org.citrusframework</groupId>
<version>4.3.1-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

There is no 4.3.1-SNAPSHOT. The snapshot versions always use the next major version. This means you need to use 4.4.0-SNAPSHOT here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I just updated it 😃

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! I think we are ready to merge then 🥳

@christophd christophd merged commit b9d9d65 into citrusframework:main Sep 2, 2024
1 check passed
@bbortt
Copy link
Contributor

bbortt commented Sep 3, 2024

thank you so much @maletic and @turing85 for this huge contribution! and all the patience 😉

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.

4 participants