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

Addressing concerns in #6 #7

Merged
merged 5 commits into from
Nov 26, 2018
Merged

Conversation

Kjames5269
Copy link
Contributor

@Kjames5269 Kjames5269 commented Nov 7, 2018

Requirements

  • Filling out the template is required. Any pull request that does not
    include enough information to be reviewed in a timely manner may be
    closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

@stustison @coyotesqrl
@bakejeyner @blen-desta

Description of the Change

Cleaning up all the pom.xml files.

Alternate Designs

Two main decisions were made while refactoring. First was to get all the generic configurations used by every child pom into the root pom. The second was to get as much use out of default configurations as possible provided by the jaxb2-maven-plugin as well as removing unnecessary tags along the way. An example of this is <forceRegenerate>. It has been deprecated as of version 0.12.1 of the plugin and claimed to be unneeded. However, there's a known issue where if multiple <execution> blocks are run with the same target directory then it will fail to generate any code past the first <execution> block if <forceRegeneration> is not present. The work around for the future to avoid using a deprecated tag is to use separate target directories which has been added in the latest commit.

Benefits

Maintainability and consistency.

Possible Drawbacks

None-

Verification Process

Shamelessly copied from here

  1. Full DDF CI build must pass
  2. Install DDF and test affected functionality
    • Create CSW sources and ensure they work correctly
    • Create WFS 1.0, 1.1, and 2.0 sources and ensure they work correctly
    • Ensure security - CAS and XACML in particular - still work
    • Ensure that metacard transformations (ingests, etc.) still work correctly

Applicable Issues

Fixes: #6

Copy link
Member

@coyotesqrl coyotesqrl left a comment

Choose a reason for hiding this comment

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

This has all been verified against DDF, I assume?

pom.xml Outdated
<plugin>
<groupId>org.jvnet.jaxb2_commons</groupId>
<artifactId>jaxb2-basics</artifactId>
<version>${jaxb2.version}</version>
Copy link
Member

Choose a reason for hiding this comment

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

✏️ Maybe we could make this a little more distinct from jaxb.version by naming it jvnet.jaxb2.version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'm also gonna check to see if there's consistency issues in naming between here and DDF with anything I added in.

@Kjames5269
Copy link
Contributor Author

Kjames5269 commented Nov 8, 2018

Yeah, I ran a full build on DDF and it passed. On top of that, I was able to ingest and query over CSW soruces. However, I did have trouble with the geoserver, but haven't verified if it's a preexisting problem on master or something these changes introduced.

pom.xml Outdated
<!-- and cannot be updated until our schemas use a newer version of JAXB2 -->
<!-- <jaxb2>0.11.0</jaxb2> also uses 2.3.0 -->
<jaxb.version>2.2.11</jaxb.version>
<jaxb2.version>0.10.0</jaxb2.version>
Copy link
Member

Choose a reason for hiding this comment

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

❓ why did this change from 0.11.1 to 0.10.0 (yeah I know the 1.11.x stuff is wrong).

Copy link
Member

Choose a reason for hiding this comment

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

Seems like we should use 0.11.1 or even 0.12.0 if possible.

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 don't actually know where 0.11.1 came from to be honest. It doesn't seem like it was being used at all in this repo. ddf-master is still using 0.9.4. @coyotesqrl might know.

Copy link
Member

Choose a reason for hiding this comment

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

I do not recall where that version came from. I would definitely prefer the latest/greatest if possible.

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 just verified that we can't update with our current schemas. The newer versions of the jaxb2-basics plugin attach what I'll call method 2's to everything. (toString2()). Our schemas call methods that are no longer created with the correct parameters giving us a type mismatch

[ERROR]     method ToString.appendFields(ObjectLocator,StringBuilder,ToStringStrategy) is not applicable
[ERROR]       (argument mismatch; ToStringStrategy2 cannot be converted to ToStringStrategy)

@bakejeyner
Copy link

Hero ✔️

@coyotesqrl coyotesqrl merged commit a5dd17f into codice:master Nov 26, 2018
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.

Consolidate generation to a single plugin version and up-to-date libraries
5 participants