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

BB: Refactor agent communication check #2388

Closed
wants to merge 1 commit into from

Conversation

cakekoa
Copy link
Contributor

@cakekoa cakekoa commented Oct 3, 2022

What does this PR do?

Fixes part of #2269.

Updated CommunicationAnalyzer to use the /api/agents and /api/machines endpoints to determine whether or not an agent communicated back to the island.

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?
  • Have you checked that you haven't introduced any duplicate code?

Testing Checklist

  • Added relevant unit tests?
  • Have you successfully tested your changes locally? Elaborate:

    Tested by running the BB tests on Jenkins

  • If applicable, add screenshots or log transcripts of the feature working
    Screen Shot 2022-10-03 at 1 00 09 PM

@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Base: 61.25% // Head: 61.24% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (0ca23cb) compared to base (eb16969).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2388      +/-   ##
===========================================
- Coverage    61.25%   61.24%   -0.01%     
===========================================
  Files          552      552              
  Lines        14429    14431       +2     
===========================================
  Hits          8838     8838              
- Misses        5591     5593       +2     
Impacted Files Coverage Δ
monkey/common/agent_event_serializers/register.py 37.50% <0.00%> (-12.50%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 1 to 8
{
"python.linting.mypyEnabled": true,
"python.linting.enabled": true,
"python.testing.cwd": "./monkey",
"python.testing.pytestArgs": [],
"python.testing.unittestEnabled": false,
"python.testing.pytestEnabled": true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Added by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep... not sure how that got added :-)

Comment on lines 21 to 22
if not self._did_agent_communicate_back(machine_ip, agent_ips):
self.log.add_entry("Agent from {} didn't communicate back".format(machine_ip))
all_monkeys_communicated = False
else:
self.log.add_entry("Monkey from {} communicated back".format(machine_ip))
self.log.add_entry("Agent from {} communicated back".format(machine_ip))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can rename and invert the logic to make it slightly more readable:

if self._agent_communicated_back():
    self.log.add_entry("Agent communicated back")
    return True

self.log.add_entry("Agent didn't communicate back")
return False

There are a bunch of variations on that, but the important thing I'm trying to get across is you can rename _did_agent_communicate_back() and invert the logic. If you do, your logic is almost an English sentence, "if agent communicated back, then...", as opposed to "If not did agent communicate back then"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not I would mind making that change 😉

Updated CommunicationAnalyzer to use the /api/agents and /api/machines
endpoints to determine whether or not an agent communicated back to the
island.
@cakekoa cakekoa force-pushed the 2269-bb-use-new-api-endpoints branch from e67dbc3 to 0ca23cb Compare October 3, 2022 18:16
mssalvatore pushed a commit that referenced this pull request Oct 3, 2022
Updated CommunicationAnalyzer to use the /api/agents and /api/machines
endpoints to determine whether or not an agent communicated back to the
island.

Resolves PR #2388
@mssalvatore
Copy link
Collaborator

Resolved by 57b4ec4

@mssalvatore mssalvatore closed this Oct 3, 2022
@mssalvatore mssalvatore deleted the 2269-bb-use-new-api-endpoints branch December 1, 2022 18:46
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