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

Support EmbeddedAnsible SCM credentials #19027

Merged
merged 1 commit into from
Aug 6, 2019
Merged

Support EmbeddedAnsible SCM credentials #19027

merged 1 commit into from
Aug 6, 2019

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jul 23, 2019 •
edited
Loading

This is an attempt (along with ManageIQ/manageiq-schema#394 ManageIQ/manageiq-schema#395) to deal with the "double ownership problem" with ConfigurationScriptSource#authentication and GitRepository. The idea is that in the model we can delegate authentication stuff to the GitRepository. Thus, we ensure a git_repository exists, then sync the authentication and other git attributes over to it.

In GitRepository model, while it might have it's own authentication via AuthenticationMixin, we also add a separate belongs_to :authentication which is a referenced authentication. This way we can support both directly owned authentications and not-owned authentications.

cc @gtanzillo @carbonin @NickLaMuro @agrare

@Fryguy
Copy link
Member Author

Fryguy commented Jul 23, 2019

Note that this PR doesn't work as is, since authentication_id is coming in during create!, and it's failing since the git_repository doesn't exist yet.

@Fryguy
Copy link
Member Author

Fryguy commented Jul 23, 2019

My original plan was to just have the caller set the git_repository.authentication directly, but then never save it, to make it "temporary", however if anything subsequently calls a git_repository method, then the authentication is lost and that feels wrong.

@miq-bot miq-bot added the wip label Jul 23, 2019
@carbonin carbonin mentioned this pull request Jul 23, 2019
Closed
@carbonin
Copy link
Member

Where and how will we handle if the SCM credential changes for a configuration script source?
Should we create an after_save hook or something to also change the one on git repositories?

carbonin
carbonin reviewed Jul 23, 2019
View reviewed changes
...models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb
@@ -8,7 +8,14 @@ class ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScri
default_value_for :scm_type, "git"
default_value_for :scm_branch, "master"

belongs_to :git_repository, :dependent => :destroy
belongs_to :git_repository, :autosave => true, :dependent => :destroy
Copy link
Member

Choose a reason for hiding this comment

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

Just caught this re: #19027 (comment)

@Fryguy Fryguy force-pushed the embedded_ansible_scm_credentials branch 3 times, most recently from 5c791c4 to 0fa2244 Compare August 2, 2019 23:14
@Fryguy Fryguy changed the title [WIP] Support EmbeddedAnsible SCM credentials Support EmbeddedAnsible SCM credentials Aug 2, 2019
@miq-bot miq-bot removed the wip label Aug 2, 2019
Fryguy
Fryguy commented Aug 2, 2019
View reviewed changes
spec/factories/configuration_script_source.rb
@@ -9,5 +9,7 @@

factory :embedded_ansible_configuration_script_source,
:parent => :configuration_script_source,
:class => "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScriptSource"
:class => "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScriptSource" do
scm_url { "https://example.com/foo.git" }
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR effectively makes scm_url a required attribute for a ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScriptSource, which I think is ok. Without this line you will consistently get a validation error on a GitRepository#url format.

@Fryguy
Copy link
Member Author

Fryguy commented Aug 2, 2019

This PR is ready-to-go codewise, but I want to add some tests around the configuration_script_source -> git_repository interactions. I did a lot of testing manually in rails console, and want to reflect those in real specs.

@Fryguy Fryguy force-pushed the embedded_ansible_scm_credentials branch from 0fa2244 to bee5eb0 Compare August 3, 2019 01:37
@Fryguy
Copy link
Member Author

Fryguy commented Aug 3, 2019

Ok specs added. I had to wrap the existing spec in a context because it always created a rugged repo, but these new tests didn't need it. So, view this with whitespace diff hidden.

@carbonin Please review.

@Fryguy Fryguy force-pushed the embedded_ansible_scm_credentials branch 3 times, most recently from e16af35 to 5cd1fb3 Compare August 3, 2019 01:45
Fryguy
Fryguy commented Aug 5, 2019
View reviewed changes
...models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb
end
end

private def sync_git_repository(git_repository = nil)
return unless name_changed? || scm_url_changed? || authentication_id_changed?
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI I really tried to DRY this up with respect to the attrs_for_sync_git_repository, by using something like changed.include_any?, but because one side has :url and the other side has :scm_url it got really ugly, so I'd prefer to just leave it this way for now.

@Fryguy Fryguy force-pushed the embedded_ansible_scm_credentials branch from 5cd1fb3 to 1043290 Compare August 5, 2019 18:16
@Fryguy Fryguy force-pushed the embedded_ansible_scm_credentials branch from 1043290 to 0deca62 Compare August 6, 2019 14:15
@Fryguy
Copy link
Member Author

Fryguy commented Aug 6, 2019

@carbonin This is ready to go.

@miq-bot
Copy link
Member

miq-bot commented Aug 6, 2019

Some comments on commit Fryguy@0deca62

spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb

  • ⚠️ - 143 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 170 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Aug 6, 2019

Checked commit Fryguy@0deca62 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 3 offenses detected

app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb

@carbonin carbonin self-assigned this Aug 6, 2019
@carbonin carbonin merged commit c262959 into ManageIQ:master Aug 6, 2019
@carbonin carbonin added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 6, 2019
simaishi pushed a commit that referenced this pull request Aug 6, 2019
@carbonin @simaishi
Merge pull request #19027 from Fryguy/embedded_ansible_scm_credentials
0afb961 
Support EmbeddedAnsible SCM credentials

(cherry picked from commit c262959)
@simaishi
Copy link
Contributor

simaishi commented Aug 6, 2019

Ivanchuk backport details:

$ git log -1
commit 0afb961d335f0bcb15d457c2d0d13029d604017b
Author: Nick Carboni <[email protected]>
Date:   Tue Aug 6 11:03:17 2019 -0400

    Merge pull request #19027 from Fryguy/embedded_ansible_scm_credentials
    
    Support EmbeddedAnsible SCM credentials
    
    (cherry picked from commit c26295952742657e6f8056ea6ba81508060095e0)

@Fryguy Fryguy deleted the embedded_ansible_scm_credentials branch August 6, 2019 19:12
@NickLaMuro NickLaMuro mentioned this pull request Oct 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbonin carbonin carbonin approved these changes

Assignees

@carbonin carbonin

Labels
changelog/yes core/embedded ansible enhancement ivanchuk/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Fryguy @carbonin @miq-bot @simaishi