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

fixes #7162 / BZ 1102763 - capsule - treat task as failed if sync times times out #4595

Merged
merged 1 commit into from Aug 27, 2014

Conversation

bbuckingham
Copy link
Member

Without this change, when an error (such as timeout) occurs attempting to
'capsule content synchronize' an environment, the synchronize task is reported
as successful. E.g. from cli, this might look like:

hammer> capsule content synchronize --id 3 --environment-id 5
[.....................................................................] [100%]
Task 6c80df66-af10-44aa-9eca-c96992148811: success

With this change, if an error occurs that is reported back from pulp as
succeeded=false, we'll treat this as an error (because it is). In
this scenario, the cli might look like:

hammer> capsule content synchronize --id 3 --environment-id 5
[.......................... ] [50%]
Task 2246bfb5-131f-4171-a7c3-6e16e3276ddd: error

module Actions
module Pulp
module Consumer
class AbstractSyncNodeTask < ::Actions::Pulp::AbstractAsyncTask
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you chose to create another class to house this? Do you see it being used by other actions? (Versus just sticking it in the sync_node.rb class?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially added it to the sync node class; however, it wasn't getting invoked. Since I saw the other cases that extended this method used separate classes, I decided to be consistent w/ them. As for reusing it later, there is a possibility, if we find other cases where the pulp structure is similar; however, we'd want to change class name for clarity at that point.

@jlsherrill
Copy link
Member

[test]

if pulp_task[:result] &&
pulp_task[:result][:details] &&
pulp_task[:result][:details][:repository] &&
pulp_task[:result][:details][:repository][:succeeded] == false
Copy link
Member

Choose a reason for hiding this comment

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

for me its pulp_task['result']['details']["node"]["succeeded"]

Copy link
Member Author

Choose a reason for hiding this comment

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

@jlsherrill, As we found in irc discussion, pulp could return either:
pulp_task['result']['details']["node"]["succeeded"]
or
pulp_task['result']['details']["repository"]["succeeded"]

As a result, in order to have a generic check, I have updated the logic to use:
pulp_task['result']['succeeded']

@jlsherrill
Copy link
Member

APJ

@bbuckingham
Copy link
Member Author

[test]

…ync times out with capsule

Without this change, when an error (such as timeout) occurs attempting to
'capsule content synchronize' an environment, the synchronize task is reported
as successful.  E.g. from cli, this might look like:

hammer> capsule content synchronize --id 3 --environment-id 5
[.....................................................................] [100%]
Task 6c80df66-af10-44aa-9eca-c96992148811: success

With this change, if an error occurs that is reported back from pulp as
succeeded=false, we'll treat this as an error (because it is).  In
this scenario, the cli might look like:

hammer> capsule content synchronize --id 3 --environment-id 5
[.....................................................................] [100%]
Task 2246bfb5-131f-4171-a7c3-6e16e3276ddd: warning
bbuckingham added a commit that referenced this pull request Aug 27, 2014
fixes #7162 / BZ 1102763 - capsule - treat task as failed if sync times times out
@bbuckingham bbuckingham merged commit 0ded175 into Katello:master Aug 27, 2014
@bbuckingham bbuckingham deleted the issue-7162 branch August 27, 2014 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants