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

Logging a disable error instead of Download error on extension update… #1616

Merged
merged 3 commits into from Aug 26, 2019

Conversation

larohra
Copy link
Contributor

@larohra larohra commented Aug 23, 2019

… failures due to disable cmd

Description

This PR is for handling the case where failure of disable.cmd in update scenario would now log from the old extension version and not the new one

Issue #


PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines


This change is Reviewable

@larohra larohra requested a review from vrdmr as a code owner August 23, 2019 00:12
@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@0025819). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1616   +/-   ##
==========================================
  Coverage           ?   66.17%           
==========================================
  Files              ?       78           
  Lines              ?    11175           
  Branches           ?     1578           
==========================================
  Hits               ?     7395           
  Misses             ?     3445           
  Partials           ?      335
Impacted Files Coverage Δ
azurelinuxagent/common/exception.py 92.85% <100%> (ø)
azurelinuxagent/ga/exthandlers.py 82.26% <100%> (ø)

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 0025819...e87624d. Read the comment docs.

@@ -436,6 +436,9 @@ def handle_ext_handler(self, ext_handler, etag):
else:
message = u"Unknown ext handler state:{0}".format(state)
raise ExtensionError(message)
except ExtensionUpdateError as e:
# Only setting the handler status here as the error has already been reported from the old version
ext_handler_i.set_handler_status(message=ustr(e), code=e.code)
Copy link
Member

Choose a reason for hiding this comment

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

this works fine now, but if we ever add/change logic in handle_ext_handler_error (which currently is the central place to handle errors) we may miss this one... consider adding a flag to handle_ext_handler_error to do/not do the logging and call it from here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'll modify the code

if ext_handler_i.version_gt(old_ext_handler_i):
ext_handler_i.update()
else:
old_ext_handler_i.update(version=ext_handler_i.ext_handler.properties.version)
Copy link
Member

Choose a reason for hiding this comment

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

wow! we support extension downgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha yes, this is at par with Windows. Not sure how much its been tested though.

Copy link
Member

Choose a reason for hiding this comment

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

would you open a work item to test it and double-check whether the extension publishers doc talks about this? i'm sure some extensions publishers will be surprised (let's also check whether the VERSION env variable is documented). Thanks!

Copy link
Contributor Author

@larohra larohra Aug 23, 2019

Choose a reason for hiding this comment

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

Opened a task. Will follow up later about this

@larohra
Copy link
Contributor Author

larohra commented Aug 23, 2019

Successful DCR run for this PR - Successful DCR Run

:param op: Which operation to run
"""
try:
if op == WALAEventOperation.Disable:
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: if you pass a lambda then you don't need the switch on operation code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Didnt know about it, will implement it!

Copy link
Member

@narrieta narrieta left a comment

Choose a reason for hiding this comment

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

Left some suggestions, but LGTM. If you update the code let me know and I can take a look.

Copy link
Contributor

@pgombar pgombar left a comment

Choose a reason for hiding this comment

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

You've already addressed my comments, thanks! LGTM

@narrieta
Copy link
Member

@larohra last set of changes LGTM

@larohra larohra merged commit 46171e6 into Azure:develop Aug 26, 2019
@larohra larohra deleted the LogDisableErrCorrectly branch September 23, 2019 23:37
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.

None yet

3 participants