-
Notifications
You must be signed in to change notification settings - Fork 41
Tower 3.3 removed result_stdout field from project_update #122
Conversation
@jameswnl Cannot apply the following label because they are not recognized: hammer/yes |
@miq-bot assign @gmcculloug |
Checked commit jameswnl@2ba210c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
def stdout(format = 'txt') | ||
api.get("#{related['stdout']}?format=#{format}").body | ||
end | ||
alias result_stdout stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a version dependent patch? Will this break older versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. I checked and this is available since 3.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still support versions older than 3.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure.
At least 3.0 reached End of Maintenance Support 2 (EOL) on 2/28/18 according to
https://access.redhat.com/support/policy/updates/ansible-tower
We started supporting Embedded Tower since 3.1. I am not sure about the external integration. Do you remember what version of Tower is the first integrated by MIQ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this change is intended to fix master and hammer branches I think we should be ok here based on the Tower EOL for 3.0.
def stdout(format = 'txt') | ||
api.get("#{related['stdout']}?format=#{format}").body | ||
end | ||
alias result_stdout stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add the alias
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the miq tower gem is depending on this alias
@gmcculloug @bdunne anything else needed for this to be merged? |
I'm good with it. @bdunne any issue merging? |
@bdunne can you create a tag to contain this fix? |
@jameswnl v0.19.0 has been released |
Fixing ManageIQ/manageiq-automation_engine#261
Related ManageIQ/manageiq-providers-ansible_tower#140