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.

Sign up for GitHub

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

Jump to bottom

Fix task name for task that create Tower project #14656

Merged
merged 1 commit into from
Apr 7, 2017
Merged

Fix task name for task that create Tower project #14656

merged 1 commit into from
Apr 7, 2017

Conversation

jameswnl
Copy link
Contributor

@jameswnl jameswnl commented Apr 5, 2017

https://bugzilla.redhat.com/show_bug.cgi?id=1439203
Fix that plus using Tower internal reference in stead of the ambiguous manager_ref

@miq-bot add_labels bug, providers/ansible_tower

bdunne
bdunne reviewed Apr 5, 2017
View reviewed changes
...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb Outdated
@@ -60,7 +60,7 @@ def update_in_provider(params)
end

def update_in_provider_queue(params)
action = "Updating #{self.class.name} with manager_ref=#{manager_ref}"
action = "Updating #{self.class.name} with Tower internal reference=#{manager_ref}"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this changing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think manager_ref wouldn't mean much to an end user as seen here.

@jameswnl
Copy link
Contributor Author

jameswnl commented Apr 6, 2017

@miq-bot add_labels fine/yes

blomquisg
blomquisg reviewed Apr 6, 2017
View reviewed changes
...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb Outdated
@@ -14,7 +14,7 @@ def create_in_provider(manager_id, params)

def create_in_provider_queue(manager_id, params)
manager = ExtManagementSystem.find(manager_id)
action = "Creating #{name} with name=#{params[:name]}"
action = "Creating #{name} with name=#{params[:name] || params['name']}"
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, I saw this in another PR. I don't like the fact that we don't know whether the param keys will be strings or symbols. That seems like an easy thing to fix. The MIQ UI and API should be sending param keys consistently.

@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

Checked commit jameswnl@c82a085 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🍰

@bdunne bdunne merged commit bb7748e into ManageIQ:master Apr 7, 2017
@bdunne bdunne added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 7, 2017
simaishi pushed a commit that referenced this pull request Apr 7, 2017
@bdunne @simaishi
Merge pull request #14656 from jameswnl/task-name
d7826fb 
Fix task name for task that create Tower project
(cherry picked from commit bb7748e)
@simaishi
Copy link
Contributor

simaishi commented Apr 7, 2017

Fine backport details:

$ git log -1
commit d7826fbae71cc541efaf4dfb7abcd2bd02a7ea4c
Author: Brandon Dunne <[email protected]>
Date:   Fri Apr 7 14:48:17 2017 -0400

    Merge pull request #14656 from jameswnl/task-name
    
    Fix task name for task that create Tower project
    (cherry picked from commit bb7748e62d8688b17ff93ee5513472593f7bf40f)

@simaishi
Copy link
Contributor

simaishi commented Apr 7, 2017

@jameswnl @bdunne I meant to ask about #14453, but pushed the backport by mistake. I think this PR depends on #14453 as app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb was added there to refactor, but that's marked as fine/no.

Either I can revert this backport, backport #14453 as well or you can fix this differently... Please let me know. I'm sorry!

@jameswnl
Copy link
Contributor Author

@simaishi yes, and now #14453 is labelled as fine/yesas well.
Thanks

simaishi pushed a commit that referenced this pull request Apr 11, 2017
@bdunne @simaishi
Merge pull request #14656 from jameswnl/task-name
a2a0f5d 
Fix task name for task that create Tower project
(cherry picked from commit bb7748e)
@simaishi
Copy link
Contributor

@jameswnl thanks. backported #14453, then this PR again to pick up the missed file.

 $ git log -1
commit a2a0f5d4a796c787b2388c39d8d608d48658d1b0
Author: Brandon Dunne <[email protected]>
Date:   Fri Apr 7 14:48:17 2017 -0400

    Merge pull request #14656 from jameswnl/task-name
    
    Fix task name for task that create Tower project
    (cherry picked from commit bb7748e62d8688b17ff93ee5513472593f7bf40f)

@jameswnl jameswnl mentioned this pull request May 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne left review comments

@blomquisg blomquisg blomquisg left review comments

Assignees
No one assigned
Labels
bug fine/backported providers/automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jameswnl @miq-bot @simaishi @blomquisg @bdunne @Fryguy