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

emulation_defender_machine_observation_state.py #197

Merged
merged 2 commits into from
Aug 16, 2023
Merged

emulation_defender_machine_observation_state.py #197

merged 2 commits into from
Aug 16, 2023

Conversation

nforsg
Copy link
Contributor

@nforsg nforsg commented Aug 16, 2023

No description provided.

@@ -131,7 +133,14 @@ def to_dict(self) -> Dict[str, Any]:

:return: a dict representation of the object
"""
d = {}
if self.ports == [] or self.ssh_connections == []:
Copy link
Owner

Choose a reason for hiding this comment

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

We probably want to allow both empty lists and None here in the Dict, i.e not raise an exception. Does mypy require an exception? We have Dict[str, Any] so it should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ja, det blir ett mypy-error om man inte tar hänsyn till None. Anledningen är för att vi måste anropa to_dict(), som inte finns om item är None. Dock kan jag göra så att jag bara lägger till None isåfall(?).

Copy link
Owner

Choose a reason for hiding this comment

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

Det blir bra, kör en if-else framför varje to_dict, och lägg till None i dicten om den är None

Copy link
Owner

Choose a reason for hiding this comment

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

Kör samma på copy(), if-else framför varje copy och lägg till None om den är None

@@ -173,6 +182,8 @@ def cleanup(self) -> None:

:return: None
"""
if self.ossec_ids_alert_counters is None:
Copy link
Owner

Choose a reason for hiding this comment

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

There is a bug in the method here. This if-statement can be removed and later down where it says ossec_ids_alert_counters.running it should be changed to ossec_ids_log_consumer_thread.running

@@ -192,6 +203,12 @@ def copy(self) -> "EmulationDefenderMachineObservationState":
"""
:return: a copy of the object
"""
if self.host_metrics is None or \
Copy link
Owner

Choose a reason for hiding this comment

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

Certain attributes of the class have type Optional. For these attributes we dont want to raise an exception, we can simply assign them to None in the object

Copy link
Owner

Choose a reason for hiding this comment

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

Also ports can be empty list, there is no problem with that

Copy link
Owner

Choose a reason for hiding this comment

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

Check if there is any type that we dont allow to be None in the class, then we check it and raise exception, otherwise we can assign it to None and Mypy should be ok

self.docker_stats_consumer_thread = None
self.snort_ids_log_consumer_thread = None
self.ossec_ids_log_consumer_thread = None
self.host_metrics_consumer_thread: Optional[HostMetricsConsumerThread] = None
Copy link
Owner

Choose a reason for hiding this comment

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

Bra

@nforsg
Copy link
Contributor Author

nforsg commented Aug 16, 2023

Har exkluderat ValueErrors på to_dict och copy och har istället integrerat None

@Limmen Limmen merged commit 6ac927c into master Aug 16, 2023
0 of 38 checks passed
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.

2 participants