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

fix(kubernetes): minor fix for getting entity type from template #3645

Merged
merged 2 commits into from
Oct 12, 2022

Conversation

PelegLi
Copy link
Contributor

@PelegLi PelegLi commented Oct 12, 2022

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

avoid exception when extracting entity type from a k8s template

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@@ -6,7 +6,7 @@ def __init__(self, report_type: str):
super().__init__(report_type)

def extract_entity_details(self, entity):
kind = entity["kind"]
kind = entity.get("kind")
Copy link
Contributor

Choose a reason for hiding this comment

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

is it ok, if it is None? not that it results in errors in other areas downstream 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only place this method is called from is here, and it means that it would not be able to get checks by entity type and that the results for this unknown entity would be empty

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, gotcha. This is more of a type hint concern, but leave it as it is, I will see how it will be, when I add type hints to kubernetes, because get_checks() actually expects a str, which it isn't, but it also doesn't fail, which makes sense from the code.

Suggested change
kind = entity.get("kind")
kind = entity.get("kind") or ""

maybe this would be better for downstream handling

Copy link
Contributor

@tronxd tronxd left a comment

Choose a reason for hiding this comment

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

If kind is None then we would not be able to run the K8 check properly since we rely on a non-null value for it. In what use case does it happen?

@PelegLi
Copy link
Contributor Author

PelegLi commented Oct 12, 2022

If kind is None then we would not be able to run the K8 check properly since we rely on a non-null value for it. In what use case does it happen?

Maybe a malformed template. If we cannot infer the entity type then we can't get the relevant checks

Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

@PelegLi PelegLi merged commit 451d74a into master Oct 12, 2022
@PelegLi PelegLi deleted the k8s-template-without-kind branch October 12, 2022 12:30
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.

3 participants