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

Improvements to brooklyn-server/rest/rest-api #1177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andreaturli
Copy link
Contributor

  • remove duplication of jackson and jackson2 on brooklyn project
  • refactor pojos to have a consistent toString, hashCode and equals
  • refactor pojos to use jackson2
  • adapt code to the new jackson2 api where needed

@ahgittin I'd appreciate your thoughts particularly on BrooklynJacksonJsonProvider as jackson api are significantly changed in that area.

Notice this is not ready to be merged as even if brooklyn-server/rest sub-modules are building fine, but karaf section need to be fixed. I'd appreciate any help from karaf experts.

@@ -0,0 +1,103 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this file now removed from source control? i know there was talk of this, but here it looks like it is being added back in.

Copy link
Member

Choose a reason for hiding this comment

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

It's been removed, but not ignored. Better ignore instead of adding it back.

Copy link
Member

Choose a reason for hiding this comment

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

Or even better - generate it somewhere under target/. Will look into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have removed the file and supposedly the pom instructions that generate it. If the file keeps reappearing we can gitignore it, and ungitignore+add it later on if we need it.

@ahgittin
Copy link
Contributor

@andreaturli - this looks really good.

@neykov is working with @CMoH on porting the REST API to CXF. that should help w any karaf issues but might hurt with conflicts elsewhere. shouldn't be too bad though, these changes are relatively self-contained. check with them on whether they can merge this in to their work.

I think

@CMoH
Copy link
Contributor

CMoH commented Jan 29, 2016

Feel free to merge before #1140. That PR already has merge conflicts, that will more time to resolve anyway. I think there's no point holding this back in the meantime.

@andreaturli
Copy link
Contributor Author

thanks @CMoH
Anyway I see problems with jenkins with this PR as well. I'll try closing and reopening it to force a new build if it was just temporary.

@andreaturli andreaturli reopened this Jan 29, 2016
@ahgittin
Copy link
Contributor

strange test failure. can't tell if it is related to this or a new intermittent one.

@andreaturli andreaturli closed this Feb 8, 2016
@andreaturli andreaturli reopened this Feb 8, 2016
@neykov
Copy link
Member

neykov commented Feb 8, 2016

@andreaturli Build failure in karaf-itest is due to feature.xml containing dependencies referencing ${jackson.version} property. The file is manually updated so you'll have to repeat the dependency changes in it as well.

@CMoH do you know of any maven config that will abort the build on discrepancy between feature.xml and the transitive maven dependencies?

@neykov
Copy link
Member

neykov commented Feb 8, 2016

And also delete dependencies.xml - not needed any more.

@CMoH
Copy link
Contributor

CMoH commented Feb 8, 2016

@neykov There are two possible ways I know of here: one is using the karaf:verify goal of karaf-maven-plugin. However, I've tested this on another project and it seems to have some problems. Here are some details: https://karaf.apache.org/manual/latest/developers-guide/karaf-maven-plugin-features-validate-descriptor.html (note that the goal was renamed to karaf:verify in the latest versions, as can be seen here: https://github.com/apache/karaf/blob/master/tooling/karaf-maven-plugin/src/main/java/org/apache/karaf/tooling/VerifyMojo.java)

The other is using the itest project to test that each feature works fine. This takes a lot of time to execute, though, and as such I'd go for the first option.

- remove JsonNode from API interfaces
- refactor consistently toStringand hashCode and equals for the domain objects
- remove deprecated code
- simplified pom dependencies
- remove com.codehaus.jackson dependency and promote com.fasterxml.jackson usage
- adjust BrooklynJacksonSerializer to use jackson 2 api
- fix web.xml for rest-api and rest-client
- update feature.xml dependencies, followin maven dependency changes
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.

5 participants