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

PAN: revamp application definitions #8178

Merged
merged 5 commits into from
Mar 23, 2022
Merged

PAN: revamp application definitions #8178

merged 5 commits into from
Mar 23, 2022

Conversation

sfraint
Copy link
Member

@sfraint sfraint commented Mar 22, 2022

Add new VS datamodel representation for Palo Alto applications.


Will separately:

  1. plumb the new representation into conversion & remove the old representation
  2. add a script / instructions for updating these.

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member Author

@sfraint sfraint left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @arifogel and @dhalperi)


projects/batfish/src/main/java/org/batfish/representation/palo_alto/application_definitions/ApplicationDefinition.java, line 37 at r1 (raw file):

  }

  @Nullable

It's not 100% clear to me what some of these properties mean, but we can fill in details as we learn more.

These properties do seem relevant to building a correct hierarchy of apps and understanding interactions, so I went ahead and added them to this new model.

Code quote:

  @Nullable
  public String getParentApp() {
    return _parentApp;
  }

  @Nullable
  public UseApplications getUseApplications() {
    return _useApplications;
  }

  @Nullable
  public UseApplications getImplicitUseApplications() {
    return _implicitUseApplications;
  }

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @arifogel)

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #8178 (e224369) into master (2c5df7f) will decrease coverage by 0.00%.
The diff coverage is 69.69%.

@@             Coverage Diff              @@
##             master    #8178      +/-   ##
============================================
- Coverage     74.49%   74.49%   -0.01%     
- Complexity    43364    43392      +28     
============================================
  Files          3376     3381       +5     
  Lines        169538   169625      +87     
  Branches      20295    20301       +6     
============================================
+ Hits         126294   126358      +64     
- Misses        33556    33573      +17     
- Partials       9688     9694       +6     
Impacted Files Coverage Δ
...pplication_definitions/ApplicationDefinitions.java 0.00% <0.00%> (ø)
..._alto/application_definitions/UseApplications.java 78.94% <78.94%> (ø)
...application_definitions/ApplicationDefinition.java 87.50% <87.50%> (ø)
...tation/palo_alto/application_definitions/Port.java 90.90% <90.90%> (ø)
...ion/palo_alto/application_definitions/Default.java 100.00% <100.00%> (ø)
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.52% <0.00%> (-1.20%) ⬇️
...batfish/src/main/java/org/batfish/main/Driver.java 48.00% <0.00%> (-0.45%) ⬇️
...ain/java/org/batfish/storage/FileBasedStorage.java 86.08% <0.00%> (-0.25%) ⬇️
...org/batfish/representation/frr/FrrConversions.java 88.53% <0.00%> (-0.24%) ⬇️
...ain/java/org/batfish/client/BfCoordWorkHelper.java 52.65% <0.00%> (-0.21%) ⬇️
... and 8 more

Copy link
Member Author

@sfraint sfraint left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @arifogel)

@sfraint sfraint merged commit 14e42dd into master Mar 23, 2022
@sfraint sfraint deleted the palo-alto-app-defs branch March 23, 2022 04:43
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