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

Schedule jobs bugfix #861

Merged
merged 3 commits into from
Dec 8, 2020
Merged

Conversation

VakarisZ
Copy link
Contributor

What does this PR do?

Fixed 2 issues:

  • Scheduled job gets created, but never deleted
  • If scheduled job is already present when monkey wants to create it, schtasks prompts a question and monkey hangs indefinitelly

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?

Testing Checklist

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

    Tested what happens if monkey job is already present
    Tested if job is created and deleted afterwards

@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #861 into develop will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #861   +/-   ##
========================================
  Coverage    60.52%   60.53%           
========================================
  Files          165      165           
  Lines         4948     4949    +1     
========================================
+ Hits          2995     2996    +1     
  Misses        1953     1953           
Impacted Files Coverage Δ
common/utils/exceptions.py 100.00% <0.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 53f3625...a777431. Read the comment docs.

@VakarisZ VakarisZ force-pushed the schedule_jobs_bugfix branch from 5356ab4 to 7abafb7 Compare October 12, 2020 13:47
reply = requests.get(url, timeout=7)
except requests.exceptions.RequestException:
logger.info("Can't get latest monkey version, probably no connection to the internet.")
raise NoInternetError
Copy link
Contributor

Choose a reason for hiding this comment

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

This might result from lack of internet connection, but also if our update server fails to respond for some reason.
I would change the exception to something like UpdateServerUnreachableException

I think the term "update server" better describes the server than "version server", but it's more important to stay consistent. So either change the rest of the variable names here to "update server" or call the exception VersionServerUnreachableException

@@ -20,3 +20,7 @@ class CredentialsNotRequiredError(RegistrationNotNeededError):

class AlreadyRegisteredError(RegistrationNotNeededError):
""" Raise to indicate the reason why registration is not required """


class VersionServerConnectionError(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, but since this is a small refactoring on separate commits, I think it's fine

@VakarisZ VakarisZ merged commit ac71a3e into guardicore:develop Dec 8, 2020
@VakarisZ VakarisZ deleted the schedule_jobs_bugfix branch December 8, 2020 14:14
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.

4 participants