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

CDPCAM-69: skip CDP CIDR validation if tunnel enabled #16

Merged
merged 2 commits into from
Aug 19, 2021
Merged

Conversation

aakanser
Copy link
Collaborator

Signed-off-by: Alper Akanser [email protected]

@codecov-commenter
Copy link

Codecov Report

Merging #16 (669f622) into dev (a47a990) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #16      +/-   ##
==========================================
+ Coverage   70.51%   70.57%   +0.05%     
==========================================
  Files          24       24              
  Lines        1048     1050       +2     
==========================================
+ Hits          739      741       +2     
  Misses        309      309              
Flag Coverage Δ
unittests 70.57% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cdpctl/validation/conftest.py 0.00% <ø> (ø)
...l/validation/infra/validate_aws_security_groups.py 88.88% <100.00%> (+0.17%) ⬆️

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 a47a990...669f622. Read the comment docs.

@@ -2,4 +2,5 @@
markers =
aws
infra
config_value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dont think this needs to be here. This is the one for unit tests not validations. It doesnt need to be in the validations.ini either because its registered here:

config.addinivalue_line(
"markers",
"config_value(path=None, value=None): "
"mark the network type a validation is target for.",
)

Copy link
Collaborator

@wonderslug wonderslug Aug 18, 2021

Choose a reason for hiding this comment

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

We might just be able to remove this whole markers section for now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dont think this needs to be here. This is the one for unit tests not validations. It doesnt need to be in the validations.ini either because its registered here:

config.addinivalue_line(
"markers",
"config_value(path=None, value=None): "
"mark the network type a validation is target for.",
)

I added that because I was seeing these when I ran pytest tests:

cdpctl/validation/infra/validate_aws_security_groups.py:138
  /workspaces/cdpctl/cdpctl/validation/infra/validate_aws_security_groups.py:138: PytestUnknownMarkWarning: Unknown pytest.mark.config_value - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/mark.html
    @pytest.mark.config_value(path="env:tunnel", value=False)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh it looks like the unit tests are finding the "tests" in the validation paths. we probably want to limit that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We might just be able to remove this whole markers section for now

we get million warnings for markers for pytest tests when we remove the markers section. There is also a pytests.ini btw, not sure why we have both of these files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

despite this? norecursedirs = cdpctl/validation/*

Copy link
Collaborator

Choose a reason for hiding this comment

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

Im guessing its erroring out when it finds them from an include actually.

I only think we need the pytest.ini. We can remove the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed pytests.ini

@wonderslug wonderslug added the bug Something isn't working label Aug 18, 2021
Signed-off-by: Alper Akanser <[email protected]>
@wonderslug wonderslug merged commit 1333b9e into dev Aug 19, 2021
@wonderslug wonderslug deleted the CDPCAM-69 branch August 19, 2021 15:48
wonderslug pushed a commit that referenced this pull request Aug 23, 2021
* CDPCAM-69: skip CDP CIDR validation if tunnel enabled

Signed-off-by: Alper Akanser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants