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

New test policies question v0 #3434

Merged
merged 3 commits into from
Mar 20, 2019
Merged

New test policies question v0 #3434

merged 3 commits into from
Mar 20, 2019

Conversation

anothermattbrown
Copy link
Contributor

No description provided.

@batfish-bot
Copy link

This change is Reviewable

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @anothermattbrown)


projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/questions/Variable.java, line 74 at r1 (raw file):

    PROTOCOL("protocol", true),
    QUESTION("question", true),
    ROUTE_SPEC("routeSpec", false),

seems like this should be bgpRouteSpec since it is BGP specific.


projects/question/src/main/java/org/batfish/question/testpolicies/TestPoliciesAnswerer.java, line 61 at r1 (raw file):

    Row row =
        row(_node, _policy, inputRoute, permit ? PERMIT : LineAction.DENY, outputRoute.build());

Inconsistent qualification of static imports.

Also, should this be a line action? (not going to bloc right now)
But what if we want a 2x2 matrix of permit/deny modified/unmodified but in enum form :)


projects/question/src/main/java/org/batfish/question/testpolicies/TestPoliciesAnswerer.java, line 75 at r1 (raw file):

            new ColumnMetadata(
                COL_POLICY_NAME, Schema.STRING, "The name of this policy", true, false),
            new ColumnMetadata(COL_INPUT_ROUTE, Schema.OBJECT, "The input route", true, false),

worth having a route schema, maybe?


projects/question/src/main/java/org/batfish/question/testpolicies/TestPoliciesQuestion.java, line 95 at r1 (raw file):

  @JsonProperty(PROP_NODES)
  public String getNode() {
    return _nodes;

I'm confused as to whether these are supposed to be filters/specs or exact matches. I don't see any filtering logic in the answerer.


projects/question/src/test/java/org/batfish/question/testpolicies/MockBatfish.java, line 66 at r1 (raw file):

import org.batfish.specifier.SpecifierContext;

final class MockBatfish implements IBatfish {

There is already IBatfishTestAdapter, where you can override specific methods you need.


questions/experimental/testPolicies.json, line 20 at r1 (raw file):

        "tags": [
            "policies",
            "trace"

"routing", "control plane" as tags. No strong opinion, just suggestions.


questions/experimental/testPolicies.json, line 32 at r1 (raw file):

                "description": "The direction of the route, with respect to the node (IN/OUT)",
                "type": "string",
                "displayName": "Direction"

allowed values? ['in', 'out']


questions/experimental/testPolicies.json, line 47 at r1 (raw file):

                    "network": { "type" : "prefix"},
                    "originatorIp": { "type": "ip"},
                    "originType": {},

why no type here? Not even string?

Copy link
Contributor Author

@anothermattbrown anothermattbrown 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: 2 of 10 files reviewed, 4 unresolved discussions (waiting on @progwriter)


projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/questions/Variable.java, line 74 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

seems like this should be bgpRouteSpec since it is BGP specific.

done.


projects/question/src/main/java/org/batfish/question/testpolicies/TestPoliciesAnswerer.java, line 61 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

Inconsistent qualification of static imports.

Also, should this be a line action? (not going to bloc right now)
But what if we want a 2x2 matrix of permit/deny modified/unmodified but in enum form :)

fixed static import.

Good idea. Added to the doc.


projects/question/src/main/java/org/batfish/question/testpolicies/TestPoliciesAnswerer.java, line 75 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

worth having a route schema, maybe?

yes, will do that in a later PR


projects/question/src/main/java/org/batfish/question/testpolicies/TestPoliciesQuestion.java, line 95 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

I'm confused as to whether these are supposed to be filters/specs or exact matches. I don't see any filtering logic in the answerer.

Exact match for v0 (see doc). Will move to specifiers soon.


projects/question/src/test/java/org/batfish/question/testpolicies/MockBatfish.java, line 66 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

There is already IBatfishTestAdapter, where you can override specific methods you need.

ooh fancy.


questions/experimental/testPolicies.json, line 20 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

"routing", "control plane" as tags. No strong opinion, just suggestions.

done.


questions/experimental/testPolicies.json, line 32 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

allowed values? ['in', 'out']

done.


questions/experimental/testPolicies.json, line 47 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

why no type here? Not even string?

done.

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #3434 into master will increase coverage by 0.04%.
The diff coverage is 92.75%.

@@             Coverage Diff             @@
##             master   #3434      +/-   ##
===========================================
+ Coverage     73.06%   73.1%   +0.04%     
- Complexity    23655   23720      +65     
===========================================
  Files          2049    2055       +6     
  Lines         99156   99369     +213     
  Branches      11893   11909      +16     
===========================================
+ Hits          72447   72643     +196     
- Misses        21393   21401       +8     
- Partials       5316    5325       +9
Impacted Files Coverage Δ Complexity Δ
...lient/src/main/java/org/batfish/client/Client.java 53.99% <0%> (-0.1%) 368 <0> (+1)
...java/org/batfish/datamodel/questions/Variable.java 94.77% <100%> (+0.06%) 33 <0> (ø) ⬇️
...sh/question/testpolicies/TestPoliciesAnswerer.java 100% <100%> (ø) 6 <6> (?)
...stion/testpolicies/TestPoliciesQuestionPlugin.java 66.66% <66.66%> (ø) 2 <2> (?)
...sh/question/testpolicies/TestPoliciesQuestion.java 96.29% <96.29%> (ø) 9 <9> (?)
...n/java/org/batfish/representation/juniper/Nat.java 88% <0%> (-12%) 8% <0%> (ø)
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.55% <0%> (-5.62%) 13% <0%> (-3%)
...org/batfish/representation/juniper/NatRuleSet.java 83.78% <0%> (-1.94%) 13% <0%> (+2%)
...g/batfish/datamodel/answers/AutoCompleteUtils.java 73.48% <0%> (-1.24%) 46% <0%> (ø)
...ain/java/org/batfish/coordinator/WorkQueueMgr.java 70.84% <0%> (-0.59%) 90% <0%> (-1%)
... and 20 more

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

:lgtm: for v0

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

@anothermattbrown anothermattbrown merged commit cfe9a6b into master Mar 20, 2019
@anothermattbrown anothermattbrown deleted the matt-routingpolicy branch March 20, 2019 23:33
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