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

New class to for determining the availability of embedded ansible #13435

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

gtanzillo
Copy link
Member

@gtanzillo gtanzillo commented Jan 10, 2017

Since Ansible will not be available initially (because it's not open source) we need a way to ask whether ansible is available from within the application so that certain features can be turned off if it's not.

This PR answers that by check if a particular directory exists. If it does, ansible is considered to be available.

https://www.pivotaltracker.com/story/show/137134397

/cc @carbonin @jrafanie

@@ -0,0 +1,24 @@
class EmbeddedAnsible
APPLIANCE_ANSIBLE_DIRECTORY = "/opt/ansible-installer"
Copy link
Member

Choose a reason for hiding this comment

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

Still not sure on this path. We can always change later, so don't want it to hold up this PR, but just wanted to mention it. cc @carbonin I was thinking /opt/awx_installer or /opt/ansible-tower-installer. ansible-installer seems like a misnomer to me.

Copy link
Member

Choose a reason for hiding this comment

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

So right now we will be installing the installer (sorry) manually so we will determine this path. When the installer is provided via rpm I assume they will install it wherever they decide so this will likely have to change either way.

@Fryguy
Copy link
Member

Fryguy commented Jan 10, 2017

I'm wondering if this should be more like the PdfGenerator in there being a NullEmbeddedAnsible. For PdfGenerator, this was useful because we had competing implementations, where none was a possible format.

The calling code does everything through this plugin, so for PDFGenerator, all of the plugins have a "generate" method, where Null does nothing. Thoughts? Is that overkill, or would that be beneficial?

end

def self.enabled?
MiqServer.my_server(true).active_roles.include?(ANSIBLE_ROLE)
Copy link
Member

Choose a reason for hiding this comment

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

Prefer MiqServer.my_server(true).has_active_role?(ANSIBLE_ROLE)

@miq-bot
Copy link
Member

miq-bot commented Jan 10, 2017

Checked commit gtanzillo@2cf595c with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. ⭐

@gtanzillo gtanzillo closed this Jan 11, 2017
@gtanzillo gtanzillo reopened this Jan 11, 2017
@@ -0,0 +1,23 @@
class EmbeddedAnsible
APPLIANCE_ANSIBLE_DIRECTORY = "/opt/ansible-installer".freeze
ANSIBLE_ROLE = "embedded_ansible".freeze
Copy link
Member

Choose a reason for hiding this comment

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

Is this knowledge something that should live elsewhere?

Is there a more "official" place where we encode the exact role names?

@carbonin
Copy link
Member

One small comment. If that's fine I would want to get this in (I have some stuff I want to put in this class for configuration and such) 😄

@gtanzillo
Copy link
Member Author

gtanzillo commented Jan 13, 2017

@Fryguy

I'm wondering if this should be more like the PdfGenerator...

I see where you're coming from but I don't see the need for it now. Do you think we could just keep the one class for now and push this logic down to a child class in the future if the need arises?

@Fryguy Fryguy merged commit 34f8505 into ManageIQ:master Jan 13, 2017
@Fryguy Fryguy added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 13, 2017
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.

5 participants