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

parser: warn user on multiple global gateway (LP: #1901836) (FR-728) #217

Merged
merged 2 commits into from Jul 13, 2021

Conversation

schopin-pro
Copy link
Contributor

@schopin-pro schopin-pro commented Jul 8, 2021

Description

It doesn't make sense for the user to have multiple global gateways. This patch introduces a consistency check at the end of the parsing stage, and only emits a warning in order not to break current configurations.

Addresses LP #1901836

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@schopin-pro
Copy link
Contributor Author

Note that #216 and this PR are in slight conflict, as merging one of them will lead to the tests failing on the other. To solve this is only a matter of changing the keywords from gateway4/6 to gateway in the tests introduced in this PR.

@schopin-pro schopin-pro requested a review from sil2100 July 8, 2021 07:58
@slyon slyon self-requested a review July 8, 2021 13:33
@slyon
Copy link
Collaborator

slyon commented Jul 8, 2021

Please mention LP: #1901836 in the PR description and title.

Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you for another great PR! Contrary to the PR description, we do actually support multiple routing tables (table: setting) – maybe you could update that accordingly.

This patch introduces a consistency check at the end of the parsing stage, and only emits a
warning in order not to break current configurations.

Yes that's good. We do not want to break existing configurations in the field. Although, arguably configuration defining multiple gateways are already broken (in a different way).

So I like your changes: Clean functions and straight forward testing.
I just want to ask for two small changes see inline comments):

  • Logical order of execution of the validate_gateway_consistency() function
  • Verbosity of the error message, please feel free to extend/modify the example I gave below

src/parse.c Outdated Show resolved Hide resolved
src/validation.c Show resolved Hide resolved
It doesn't make sense for the user to have multiple global gateways.
This patch introduces a consistency check at the end of the parsing
stage, and only emits a warning in order not to break current
configurations.
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2021

Codecov Report

Merging #217 (dd7e09e) into master (2fd3fc2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #217   +/-   ##
=======================================
  Coverage   99.00%   99.01%           
=======================================
  Files          55       55           
  Lines        9178     9200   +22     
=======================================
+ Hits         9087     9109   +22     
  Misses         91       91           
Impacted Files Coverage Δ
src/parse.c 100.00% <100.00%> (ø)
src/validation.c 99.48% <100.00%> (+0.04%) ⬆️
tests/generator/test_errors.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd3fc2...dd7e09e. Read the comment docs.

@slyon slyon changed the title parser: warn user on multiple global gateway (FR-728) parser: warn user on multiple global gateway (LP: #1901836) (FR-728) Jul 13, 2021
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you Simon, LGTM! 👀

I've cleaned up some whitespace in the warning message log and I think it's ready for merging, independently of #216

@slyon slyon merged commit 54283a0 into canonical:master Jul 13, 2021
@schopin-pro schopin-pro deleted the gateway_uniqueness branch July 21, 2021 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants