Junos: Warn when add/setting community with no non-wildcard members

[SirasornT]

warnIllegalNamedCommunitiesUsedForSet never puts out a warning because getOrCreateNamedCommunitiesUsedForSet() never has a community with no literal member (filtered out in PsThenCommunitySet).

This PR adds fatal red flag warning when trying to add or set a community with no literal member.

[batfish-bot]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.16%. Comparing base (1447281) to head (bc30abb).
Report is 50 commits behind head on master.

Files with missing lines	Patch %	Lines
...ish/representation/juniper/PsThenCommunitySet.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9355      +/-   ##
==========================================
- Coverage   73.16%   73.16%   -0.01%     
==========================================
  Files        3321     3321              
  Lines      169206   169222      +16     
  Branches    20112    20118       +6     
==========================================
+ Hits       123808   123810       +2     
- Misses      36292    36300       +8     
- Partials     9106     9112       +6     

Files with missing lines	Coverage Δ
...h/representation/juniper/JuniperConfiguration.java	86.78% <ø> (+0.07%)	⬆️
...ish/representation/juniper/PsThenCommunityAdd.java	84.21% <100.00%> (+5.63%)	⬆️
...ish/representation/juniper/PsThenCommunitySet.java	88.88% <80.00%> (-3.42%)	⬇️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SLarkworthy]

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved

[SirasornT]

Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SirasornT)

[dhalperi]

projects/batfish/src/main/java/org/batfish/representation/juniper/PsThenCommunityAdd.java line 29 at r1 (raw file):

    if (!c.getCommunitySets().containsKey(_name)) {
      // undefined reference; or not converted because it contains only regexes
      juniperVendorConfiguration

Why is this not using the Warnings argument?

[dhalperi]

projects/batfish/src/main/java/org/batfish/representation/juniper/PsThenCommunityAdd.java line 29 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Why is this not using the Warnings argument?

(throughout)

[dhalperi]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SirasornT)

---

projects/batfish/src/test/resources/org/batfish/grammar/juniper/testconfigs/juniper-community line 1 at r1 (raw file):

#

Two nits:

1. Consider renaming this file from juniper-community to something like community-literals-warnings for example that indicates what it's testing.

2. Consider adding (just for documentation sake) a delete line in the legal section. It's okay to delete from a wildcard, right? :)

[dhalperi]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SirasornT)

---

projects/batfish/src/test/resources/org/batfish/grammar/juniper/testconfigs/juniper-community line 1 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Two nits:

1. Consider renaming this file from juniper-community to something like community-literals-warnings for example that indicates what it's testing.

2. Consider adding (just for documentation sake) a delete line in the legal section. It's okay to delete from a wildcard, right? :)

(on 1: the juniper- is redundant given path, tho the existing patterns are obviously mixed)

[dhalperi]

projects/batfish/src/main/java/org/batfish/representation/juniper/PsThenCommunityAdd.java line 28 at r1 (raw file):

      Warnings warnings) {
    if (!c.getCommunitySets().containsKey(_name)) {
      // undefined reference; or not converted because it contains only regexes

This comment says this can also happen for an undefined reference. is that true? If so, it's wrong to warn here unconditionally.

[SirasornT]

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

---

projects/batfish/src/main/java/org/batfish/representation/juniper/PsThenCommunityAdd.java line 28 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

This comment says this can also happen for an undefined reference. is that true? If so, it's wrong to warn here unconditionally.

Yes this is true. I have added a condition to prevent exclude undefined reference for this warning

---

projects/batfish/src/main/java/org/batfish/representation/juniper/PsThenCommunityAdd.java line 29 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

(throughout)

Done.

---

projects/batfish/src/test/resources/org/batfish/grammar/juniper/testconfigs/juniper-community line 1 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

(on 1: the juniper- is redundant given path, tho the existing patterns are obviously mixed)

Done.

[dhalperi]

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @SirasornT and @SLarkworthy)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Configuration.java line 495 at r2 (raw file):

  @JsonIgnore
  public @Nonnull Set<String> getWildcardCommunitySets() {
    return _wildcardCommunitySets;

We can't put Junos-specific stuff in the VI (vendor-independent) model. Suggest talking to @SLarkworthy or @progwriter about approach here

[SirasornT]

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @dhalperi and @SLarkworthy)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Configuration.java line 495 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

We can't put Junos-specific stuff in the VI (vendor-independent) model. Suggest talking to @SLarkworthy or @progwriter about approach here

Will do

[SirasornT]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @dhalperi, @progwriter, and @SLarkworthy)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Configuration.java line 495 at r2 (raw file):

Previously, SirasornT (Sirasornt) wrote…

Will do

Done.

[dhalperi]

Reviewed 2 of 7 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SirasornT)