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

infrastructure for Eclipse external annotations #1560

Merged
merged 6 commits into from Jun 7, 2018

Conversation

arifogel
Copy link
Member

@arifogel arifogel commented Jun 1, 2018

@dhalperi Please review pom changes

@arifogel arifogel requested a review from dhalperi June 1, 2018 21:32
@batfish-bot
Copy link

This change is Reviewable

@dhalperi
Copy link
Member

dhalperi commented Jun 1, 2018

Review status: 108 of 120 files reviewed at latest revision, all discussions resolved.


a discussion (no related file):
Reviewing this, I have become very confused. Why does any of this, at all, go in Batfish? Can't you just build yourself an annotation database and configure Eclipse to use it?

Nothing about eea-parent seems related to Batfish.


projects/pom.xml, line 85 at r1 (raw file):

    <!-- Annotation path for Eclipse -->
    <m2e.jdt.annotationpath>${project.basedir}/../eea-parent/eea-all/target/eea-all-${version}.jar</m2e.jdt.annotationpath>

this seems fishy. Should not ever see any .. in these.


Comments from Reviewable

@arifogel
Copy link
Member Author

arifogel commented Jun 4, 2018

Review status: 7 of 16 files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

Reviewing this, I have become very confused. Why does any of this, at all, go in Batfish? Can't you just build yourself an annotation database and configure Eclipse to use it?

Nothing about eea-parent seems related to Batfish.

I have removed eea-parent in interest of proper separation of concerns. We lost some functionality, but nothing critical. See comment in parent pom.


projects/pom.xml, line 85 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

this seems fishy. Should not ever see any .. in these.

Fixed


Comments from Reviewable

@arifogel arifogel force-pushed the ari-eclipse-external-annotations branch from 26f70d1 to 49edf0d Compare June 4, 2018 16:34
@dhalperi
Copy link
Member

dhalperi commented Jun 6, 2018

Review status: 8 of 16 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


projects/allinone/pom.xml, line 142 at r3 (raw file):

      <groupId>org.lastnpe.eea</groupId>
      <artifactId>jdk-eea</artifactId>
    </dependency>

this adds these jars as compile-time dependencies for all our code -- this doesn't seem right, as they shouldn't be shipped with our jars.

Is there a proper way to integrate this just for eclipse IDE work (using, say, m2eclipse config for example)?


Comments from Reviewable

@arifogel
Copy link
Member Author

arifogel commented Jun 6, 2018

Review status: 8 of 16 files reviewed at latest revision, 2 unresolved discussions.


projects/allinone/pom.xml, line 142 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

this adds these jars as compile-time dependencies for all our code -- this doesn't seem right, as they shouldn't be shipped with our jars.

Is there a proper way to integrate this just for eclipse IDE work (using, say, m2eclipse config for example)?

Could not figure out how to do it in eclipse-specific fashion.
However I changed to prodded scope so it doesn't get built into the jars.


Comments from Reviewable

@arifogel
Copy link
Member Author

arifogel commented Jun 6, 2018

Review status: 8 of 16 files reviewed at latest revision, 2 unresolved discussions.


projects/allinone/pom.xml, line 142 at r3 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Could not figure out how to do it in eclipse-specific fashion.
However I changed to prodded scope so it doesn't get built into the jars.

provided


Comments from Reviewable

@codecov
Copy link

codecov bot commented Jun 6, 2018

Codecov Report

Merging #1560 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1560      +/-   ##
============================================
+ Coverage     66.49%   66.53%   +0.04%     
- Complexity    14794    14817      +23     
============================================
  Files          1361     1365       +4     
  Lines         72204    72343     +139     
  Branches       9567     9583      +16     
============================================
+ Hits          48014    48137     +123     
- Misses        20512    20530      +18     
+ Partials       3678     3676       -2
Impacted Files Coverage Δ Complexity Δ
...epresentation/palo_alto/PaloAltoConfiguration.java 92.85% <0%> (ø) 5% <0%> (?)
...rammar/palo_alto/PaloAltoConfigurationBuilder.java 44.82% <0%> (ø) 4% <0%> (?)
...ammar/palo_alto/PaloAltoControlPlaneExtractor.java 92.85% <0%> (ø) 3% <0%> (?)
...fish/grammar/palo_alto/PaloAltoCombinedParser.java 100% <0%> (ø) 3% <0%> (?)
...col/src/main/java/org/batfish/role/InferRoles.java 94.16% <0%> (+0.41%) 70% <0%> (+1%) ⬆️
...src/main/java/org/batfish/coordinator/WorkMgr.java 62.03% <0%> (+0.67%) 125% <0%> (ø) ⬇️
...src/main/java/org/batfish/coordinator/PoolMgr.java 68.81% <0%> (+3.22%) 15% <0%> (ø) ⬇️
...a/org/batfish/job/ParseVendorConfigurationJob.java 62.59% <0%> (+4.07%) 21% <0%> (+5%) ⬆️
...g/batfish/coordinator/authorizer/DbAuthorizer.java 54.43% <0%> (+8.28%) 15% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24b23f7...272591b. Read the comment docs.

@dhalperi
Copy link
Member

dhalperi commented Jun 6, 2018

Review status: 12 of 16 files reviewed at latest revision, 1 unresolved discussion.


a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

I have removed eea-parent in interest of proper separation of concerns. We lost some functionality, but nothing critical. See comment in parent pom.

How did you choose provided vs optional? If optional works in Eclipse, that might be more correct.

provided means that we expect at runtime the jar will be there.


Comments from Reviewable

@arifogel
Copy link
Member Author

arifogel commented Jun 7, 2018

Review status: 12 of 16 files reviewed at latest revision, 1 unresolved discussion.


a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

How did you choose provided vs optional? If optional works in Eclipse, that might be more correct.

provided means that we expect at runtime the jar will be there.

From my read of:
https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.htm
It sounds like we do want it to be an optional dependency, but that optional-ness is orthogonal to scope. I just chose provided scope so that the eaa files would not be included in the bundle.


Comments from Reviewable

@dhalperi dhalperi merged commit f270158 into master Jun 7, 2018
@dhalperi dhalperi deleted the ari-eclipse-external-annotations branch June 7, 2018 04:19
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.

None yet

3 participants