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

Cumulus and L1 topology: Canonicalize hostnames to lowercase #4809

Merged
merged 11 commits into from
Sep 24, 2019

Conversation

corinaminer
Copy link
Contributor

No description provided.

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@corinaminer corinaminer changed the title Canonicalize Cumulus hostnames to lowercase Cumulus and L1 topology: Canonicalize hostnames to lowercase Sep 24, 2019
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 1 of 1 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @corinaminer)


projects/batfish/src/test/java/org/batfish/dataplane/EvpnCumulusTest.java, line 33 at r2 (raw file):

    final String leaf1 = "Leaf1";
    final String leaf2 = "Leaf2";

why not fix these variables instead?


projects/batfish/src/test/java/org/batfish/dataplane/EvpnType5CumulusTest.java, line 57 at r2 (raw file):

        dp.getRibs();
    String vrf1 = "vrf1";
    final ImmutableList<String> leafs = ImmutableList.of("Leaf1", "Leaf2", "Leaf3", "Leaf4");

why not fix these vars instead?


projects/batfish-common-protocol/src/main/java/org/batfish/common/topology/Layer1Topology.java, line 30 at r2 (raw file):

Layer1Edge

Layer1Edge instead?

Copy link
Contributor Author

@corinaminer corinaminer 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: 1 of 5 files reviewed, all discussions resolved (waiting on @dhalperi)


projects/batfish/src/test/java/org/batfish/dataplane/EvpnCumulusTest.java, line 33 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
    final String leaf1 = "Leaf1";
    final String leaf2 = "Leaf2";

why not fix these variables instead?

Didn't realize loading the file would be case insensitive. Done


projects/batfish/src/test/java/org/batfish/dataplane/EvpnType5CumulusTest.java, line 57 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

why not fix these vars instead?

Done


projects/batfish-common-protocol/src/main/java/org/batfish/common/topology/Layer1Topology.java, line 30 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
Layer1Edge

Layer1Edge instead?

Layer1Node is the lowest-level owner of L1 hostnames, canonicalized there instead.

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 5 of 5 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@corinaminer corinaminer 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: 5 of 6 files reviewed, all discussions resolved


projects/batfish-common-protocol/src/test/java/org/batfish/common/topology/TopologyUtilTest.java, line 1359 at r4 (raw file):

    _cb.setConfigurationFormat(ConfigurationFormat.CISCO_IOS);
    Configuration c1 = _cb.setHostname("c1").build();
    Configuration c2 = _cb.setHostname("c2").build();

this is a workaround to avoid using generated hostnames, which contain a capital letter, causing a mismatch between c1.getHostname() and the hostnames in L1 topology. Should probably stop putting a capital letter in generated hostnames, but that will probably break a bunch more tests, and did not want to overextend this PR.

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 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@corinaminer corinaminer 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: 4 of 7 files reviewed, all discussions resolved (waiting on @dhalperi)


projects/batfish/src/test/java/org/batfish/dataplane/EvpnCumulusTest.java, line 33 at r2 (raw file):

Previously, corinaminer (Corina Miner) wrote…

Didn't realize loading the file would be case insensitive. Done

Looks like buildkite is not case flexible for loading files :/

@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #4809 into master will decrease coverage by <.01%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master    #4809      +/-   ##
============================================
- Coverage     75.47%   75.47%   -0.01%     
+ Complexity    28646    28641       -5     
============================================
  Files          2283     2283              
  Lines        112552   112552              
  Branches      13518    13518              
============================================
- Hits          84947    84946       -1     
+ Misses        21117    21113       -4     
- Partials       6488     6493       +5
Impacted Files Coverage Δ Complexity Δ
...presentation/cumulus/CumulusNcluConfiguration.java 95% <0%> (-0.13%) 203 <1> (ø)
...n/java/org/batfish/common/topology/Layer1Node.java 71.87% <100%> (ø) 12 <0> (ø) ⬇️
...g/batfish/datamodel/acl/IpAccessListLineIndex.java 33.33% <0%> (-5.56%) 4% <0%> (-1%)
...java/org/batfish/symbolic/state/EdgeStateExpr.java 73.91% <0%> (-4.35%) 10% <0%> (-1%)
...rc/main/java/org/batfish/datamodel/flow/Trace.java 87.09% <0%> (-3.23%) 12% <0%> (-1%)
...rg/batfish/dataplane/ibdp/EigrpRoutingProcess.java 93.12% <0%> (-0.39%) 69% <0%> (-1%)
...col/src/main/java/org/batfish/role/InferRoles.java 90.9% <0%> (-0.38%) 65% <0%> (-1%)
...atfish/src/main/java/org/batfish/main/Batfish.java 75.93% <0%> (+0.3%) 286% <0%> (+1%) ⬆️

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 3 of 3 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@corinaminer corinaminer merged commit 05df55d into master Sep 24, 2019
@corinaminer corinaminer deleted the cumulus-hostnames branch September 24, 2019 19:53
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

4 participants