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

Fix handling of successful del-network #3458

Merged
merged 2 commits into from
Mar 23, 2019

Conversation

agember
Copy link
Contributor

@agember agember commented Mar 22, 2019

Running del-network when a network exists returns 200 OK not 204 OK. Thus, an error message should only be printed if the status code is not 200 OK.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #3458 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #3458      +/-   ##
============================================
- Coverage     73.13%   73.09%   -0.04%     
- Complexity    23787    23830      +43     
============================================
  Files          2062     2073      +11     
  Lines         99540    99747     +207     
  Branches      11935    11946      +11     
============================================
+ Hits          72800    72912     +112     
- Misses        21416    21493      +77     
- Partials       5324     5342      +18
Impacted Files Coverage Δ Complexity Δ
...ain/java/org/batfish/client/BfCoordWorkHelper.java 28.64% <0%> (ø) 29 <0> (ø) ⬇️
...c/main/java/org/batfish/dataplane/rib/RibTree.java 78.72% <0%> (-10.64%) 23% <0%> (-1%)
...g/batfish/datamodel/acl/IpAccessListLineIndex.java 33.33% <0%> (-5.56%) 4% <0%> (-1%)
...org/batfish/datamodel/flow/BidirectionalTrace.java 81.81% <0%> (-2.28%) 14% <0%> (-1%)
...col/src/main/java/org/batfish/role/InferRoles.java 90.62% <0%> (-1.57%) 68% <0%> (-2%)
...in/java/org/batfish/dataplane/rib/AbstractRib.java 95.52% <0%> (-1.5%) 33% <0%> (-1%)
...in/java/org/batfish/common/CompletionMetadata.java 98.7% <0%> (-1.3%) 26% <0%> (+1%)
...src/main/java/org/batfish/coordinator/PoolMgr.java 64.04% <0%> (-1.13%) 15% <0%> (-1%)
...g/batfish/datamodel/answers/AutoCompleteUtils.java 72.87% <0%> (-0.76%) 47% <0%> (+1%)
...tfish/representation/cisco/CiscoConfiguration.java 83.48% <0%> (-0.34%) 520% <0%> (-4%)
... and 32 more

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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @agember)


projects/batfish-client/src/main/java/org/batfish/client/BfCoordWorkHelper.java, line 252 at r1 (raw file):

              .delete();

      if (response.getStatus() != Status.OK.getStatusCode()) {

Nice catch!

It looks like this is entirely the wrong paradigm, and this should be:

if (response.getStatus().getFamily() != Family.SUCCESSFUL) {

(See: https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Response.Status.html#getFamily--)

Then, the code will be robust to minor output changes on the server side that are still within contract. Would you make that change?

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.

:lgtm: thanks!

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dhalperi dhalperi merged commit 551ffa2 into batfish:master Mar 23, 2019
@agember agember deleted the aaron-fix-del-network branch June 6, 2019 16:56
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