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

Add plugin dir to developer jetty:run classpath for plugin dev #8033

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mlsorensen
Copy link
Contributor

Description

This PR is a simple change to allow adding plugins to CloudStack when running in developer mode. Simply drop plugin jar into client/target/plugins before launching mvn -pl :cloud-client-ui jetty:run.

Also bumping the version of jetty-maven-plugin to the same as main jetty server.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #8033 (95896d9) into main (2e9b3d8) will decrease coverage by 14.66%.
Report is 74 commits behind head on main.
The diff coverage is n/a.

@@              Coverage Diff              @@
##               main    #8033       +/-   ##
=============================================
- Coverage     28.57%   13.92%   -14.66%     
+ Complexity    29784    10820    -18964     
=============================================
  Files          5100     3105     -1995     
  Lines        358565   303890    -54675     
  Branches      52316    52337       +21     
=============================================
- Hits         102464    42305    -60159     
- Misses       241968   256123    +14155     
+ Partials      14133     5462     -8671     
Flag Coverage Δ
simulator-marvin-tests ?
uitests 5.37% <ø> (+0.50%) ⬆️
unit-tests 14.74% <ø> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3272 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -154,7 +154,7 @@
<cs.jaxws.version>2.3.2-1</cs.jaxws.version>
<cs.jersey-client.version>2.26</cs.jersey-client.version>
<cs.jetty.version>9.4.51.v20230217</cs.jetty.version>
<cs.jetty-maven-plugin.version>9.4.27.v20200227</cs.jetty-maven-plugin.version>
<cs.jetty-maven-plugin.version>9.4.51.v20230217</cs.jetty-maven-plugin.version>
Copy link
Member

Choose a reason for hiding this comment

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

The 9.4.27 was a known/working version, I'm not sure if with more recent working the jetty-maven-plugin works @mlsorensen have you tested it locally (see most Github actions jobs are failing)

Copy link
Contributor Author

@mlsorensen mlsorensen Oct 4, 2023

Choose a reason for hiding this comment

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

It worked locally, and it's the version of jetty that is bundled with the production cloudstack, but perhaps it is incompatible with the testing/PR process in some way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the new versions of jetty may be more aggressive at scanning? I see more duplicate classes found, seems to be searching current directory as well as finding them in client/target/cloud-client-ui jar. Will try a few things...

Copy link
Contributor Author

@mlsorensen mlsorensen Oct 4, 2023

Choose a reason for hiding this comment

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

Ok I think maybe I found the issue @rohityadavcloud. Newer jetty-maven-plugin versions add the pom dependencies to classpath automatically, with older jetty we have defined extraClasspath and add a built cloud-client-ui.jar location. This results in a lot of duplicate classes discovered. Also notice that due to using the pom dependencies for building classpath, without specifying -Psimulator the jetty:run doesn't have simulator available which breaks the simulator CI build.

We can add explicit simulator dependency for the jetty-maven-plugin, or we can add -Psimulator to the CI (but I don't know how to do the latter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, I'm not confident that I know exactly what's going on here, but I think the same PR in jetty that I need for this feature is probably the same one that cloudstack's use of jetty:run is incompatible with. Or certainly the very next release after this.

May need to resolve the underlying reason why we can't upgrade to newer jetty-maven-plugin before we consider adding a plugin dir for devs.

@DaanHoogland DaanHoogland reopened this Oct 4, 2023
@rohityadavcloud
Copy link
Member

@mlsorensen smoketests (github actions) are still failing

@rohityadavcloud
Copy link
Member

@mlsorensen I think this is incompatible version and may require some additional changes to make them work. Does it work for you locally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants