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

Add SCM options and SCMCredential to Ansible Tower Automation Manager ConfigurationScriptSource #13776

Merged
merged 1 commit into from
Feb 15, 2017

Conversation

jameswnl
Copy link
Contributor

@jameswnl jameswnl commented Feb 6, 2017

We need to keep these information so that when UI provide edit, there'll be existing values to be displayed.

@jameswnl
Copy link
Contributor Author

jameswnl commented Feb 6, 2017

@miq-bot add_labels wip, enhancement, providers/ansible_provider

@miq-bot
Copy link
Member

miq-bot commented Feb 6, 2017

@jameswnl Cannot apply the following label because they are not recognized: providers/ansible_provider

@jameswnl
Copy link
Contributor Author

jameswnl commented Feb 6, 2017

@miq-bot add_label providers/ansible_tower

@jameswnl jameswnl force-pushed the tower-project branch 2 times, most recently from 529b9d2 to 12ae1f0 Compare February 6, 2017 20:02
@jameswnl jameswnl changed the title [WIP] Add SCM options and SCMCredential to Ansible Tower Automation Manager ConfigurationScriptSource Add SCM options and SCMCredential to Ansible Tower Automation Manager ConfigurationScriptSource Feb 6, 2017
@jameswnl
Copy link
Contributor Author

jameswnl commented Feb 6, 2017

@miq-bot remove_label wip

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM, just need schema review from @Fryguy

@@ -1,4 +1,5 @@
class ConfigurationScriptSource < ApplicationRecord
has_many :configuration_script_payloads
belongs_to :authentications
Copy link
Member

Choose a reason for hiding this comment

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

Typically a belongs_to is singular. Does this actually work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me write a test to find out :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. I've also deleted the spec file which is testing relationships only.

@miq-bot
Copy link
Member

miq-bot commented Feb 15, 2017

Checked commit jameswnl@3541b75 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 1 offense detected

spec/lib/miq_automation_engine/service_models/miq_ae_service_manageiq-providers-ansible_tower-automation_manager-scm_credential_spec.rb

  • ❗ - Line 1, Col 1 - Style/FileName - The name of this source file (miq_ae_service_manageiq-providers-ansible_tower-automation_manager-scm_credential_spec.rb) should use snake_case.

expect(payloads[0].configuration_script_source).to eq(configuration_script_source)
end
end
end
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 deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are just testing rails

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok...I remember that discussion, but I thought it was from a different PR. This is fine.

@Fryguy Fryguy added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 15, 2017
@Fryguy Fryguy merged commit 9e2b41d into ManageIQ:master Feb 15, 2017
@jameswnl jameswnl deleted the tower-project branch February 15, 2017 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants