-
Notifications
You must be signed in to change notification settings - Fork 200
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
Replace foreman_installer role #516
Conversation
a0ae2f9
to
6f332b0
Compare
While I'm open to the idea, this will take some time to review I think. We should also look at the bigger picture: is a submodule the way to go? @ehelms also started https://github.com/theforeman/foreman-ansible-modules so perhaps there's more we can converge on in terms of using external repositories and how we do that. |
I will also take a bit of time to review and consider this. I do have some
preliminary questions and thoughts.
On Tue, Aug 15, 2017 at 10:13 AM, sean797 ***@***.***> wrote:
This replaces foreman_installer role with https://github.com/sean797/
ansible-role-foreman_installer (https://galaxy.ansible.com/
sean797/foreman_installer/)
A couple of benefits this provides:
- Generates Proxy Certs on the fly (replaces the current
foreman_proxy_content role)
Can you better explain "on the fly" and the benefits? As well, what is the
benefit of replacing the narrowly focused foreman_proxy_content role?
- Automatically get oauth key's and secrets from the Foreman or
Katello Server.
As opposed to?
- Supports HA Foreman/Katello's
Can you talk about this more?
I've added it via git submodule as its also on Galaxy and I think it makes
sense to not host it in forklift. I'm happy to define it in
requirements.yml and/or move it to theforeman GitHub org.
Why do you think it makes sense not to host it in Forklift? I am not a huge
fan of submodules as they go out of date easily.
…--
Eric D. Helms
Red Hat Engineering
|
I agree with your points about submodules, I opted for that because A flexible Proxy certs tar generation: HA support:
With the current role you have to do something like https://github.com/theforeman/forklift/blob/master/pipelines/pipeline_katello_nightly.yml#L61 supply the options you get, whereas this role will do that for you providing you supply The main reason behind this is I've created a flexible foreman_installer role, and it seem unnecessary for the me or the project to maintain both (and I want to share it ;-) ) |
On Tue, Aug 15, 2017 at 3:38 PM, sean797 ***@***.***> wrote:
I agree with your points about submodules, I opted for that because
requirements.yml is in .gitignore and defining roles in requirements.yml
would require another step to pull down the role(s). Not the end of the
world I guess, either way works. There are pros and cons of both.
Yea - but you do it once and you're good. Not sure why we set it to be
ignored.
A flexible foreman_installer Ansible role is missing on Galaxy, so I want
to be able to publish there. AFAIK its 1 git repo -> 1 role mapping
(requiring a /meta/main.yml file), hosting it in Forklift would result in
problems importing it into Galaxy I thing.
So this gets us to tension between how Ansible chooses to share and manage
roles and how we treat and maintain Forklift. I find this same issue to be
true with puppet. Creating stand-alone roles, to facilitate the
Galaxy/forge workflow, increases development complexity. If I need to fix
an issue, I now have to make 1 or more commits across repositories, and
hope their maintainers are on point (note I am speaking in the general, not
speaking against you as a maintainer). So it becomes balancing the value of
a stand alone role with development complexity. This type of change also
moves us to a third party maintainer which has pros and cons. For example,
we now have one less thing we are maintaining, but we have to trust that
updates to it do not break out primary workflows and tend to match our
standards.
*Proxy certs tar generation:*
You provide a generate_proxy_certs_from var and it will go and generate
the certs, copy them over, make all the oauth stuff correct and do the
install.
I'm on the fence, as I can see your point but in some ways the cert
generation (and collection of answers such as oauth, etc) is separate from
the running of the installer itself. We've pushed for small, single minded
roles in Forklift to allow easier composition. I'll look at the code a bit.
*HA support:*
See https://github.com/sean797/ansible-role-foreman_
installer#katello-cluster-with-custom-certificates
Basically it can setup oauth, password other answer,
/etc/foreman/encryption_key.rb so they're the same.
- Automatically get oauth key's and secrets from the Foreman or
Katello Server.
As opposed to?
With the current role you have to do something like
https://github.com/theforeman/forklift/blob/master/
pipelines/pipeline_katello_nightly.yml#L61 supply the options you get,
whereas this role will do that for you providing you supply
generate_proxy_certs_from var. See
That aspect does simplify things quite a bit for what needs to be defined.
These are the things that make me think we need an installer module instead
of a role, so it can be called from any location without having to overload
a role for all cases or redefine calls to the installer.
The main reason behind this is I've created a flexible foreman_installer
role, and it seem unnecessary for the me or the project to maintain both
(and I want to share it ;-) )
There is a wrinkle I need to think about as well, with respect to plugins
or other projects using Forklift and its roles and expectations on them. In
that if we change this, and the "API" is not 1:1 we can break others :/
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#516 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAR586Lop7SYZJhnJyv50Gfjm0MpuIH3ks5sYfPEgaJpZM4O3owG>
.
--
Eric D. Helms
Red Hat Engineering
Ph.D. Student - North Carolina State University
|
pipelines/pipeline_katello_32.yml
Outdated
@@ -19,9 +19,6 @@ | |||
katello_repositories_version: 3.2 | |||
foreman_repositories_version: 1.13 | |||
katello_repositories_use_koji: true | |||
foreman_installer_skip_installer: true | |||
foreman_installer_additional_packages: | |||
- foreman-installer-katello |
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 remove these? They help to optimize runs.
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 role I'm proposing replacing this with doesn't have a foreman_installer_skip_installer
or similar var, I think having a foreman_installer role that dosen't actually run the installer is kind of pointless.
If we want to just install packages, maybe we should extend the update_os_packages
role or create a os_packages
role.
pipelines/pipeline_katello_32.yml
Outdated
- "--foreman-admin-password {{ foreman_installer_admin_password }}" | ||
foreman_installer_additional_packages: | ||
- katello | ||
installer_scenarios_answers: |
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.
Shouldn't this be foreman_installer_scenarios_answers
?
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.
👍 I probably should rename that var...
pipelines/pipeline_katello_32.yml
Outdated
- katello | ||
installer_scenarios_answers: | ||
foreman: | ||
admin_password: changeme |
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 set this this way in contrast to the foreman_installer_options
? What advantage is there to having two ways to configure the installer?
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.
This makes the role idempotent, if you modify foreman_installer_options
you have to run the installer to make sure its applied (everytime), wheras this method allows you to manage the answer file and run the installer as a handler (only if something changes). https://github.com/sean797/ansible-role-foreman_installer/blob/master/templates/scenario-answers.yaml.j2
pipelines/pipeline_katello_32.yml
Outdated
installer_scenarios_answers: | ||
foreman: | ||
admin_password: changeme | ||
foreman_installer_pkg: katello |
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.
This option, that replaced the previous was an array, while this is a single option -- why the change?
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.
I never had any need to install more than 1 package, I'll happily change it to take an array if required. I can't see anywhere where is actually need?
pipelines/pipeline_katello_32.yml
Outdated
roles: | ||
- foreman_installer | ||
|
||
- hosts: pipeline-proxy-3.2-centos7 | ||
become: yes | ||
vars: | ||
foreman_proxy_content_server: pipeline-katello-3.2-centos7 | ||
generate_proxy_certs_from: pipeline-katello-3.2-centos7.example.com |
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.
This is missing the foreman_installer
namespace. This will also break because you are baking in the domain even though the domain is dynamically generated when using Vagrant.
playbooks/bats_pipeline_nightly.yml
Outdated
foreman: | ||
admin_password: changeme | ||
katello: | ||
enable_ostree: true |
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.
Indentation is off here
Some of my review comments apply in multiple places, so I did not repeat myself for the sake of us all. |
On Tue, Aug 15, 2017 at 5:44 PM, sean797 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pipelines/pipeline_katello_32.yml
<#516 (comment)>:
> @@ -19,9 +19,6 @@
katello_repositories_version: 3.2
foreman_repositories_version: 1.13
katello_repositories_use_koji: true
- foreman_installer_skip_installer: true
- foreman_installer_additional_packages:
- - foreman-installer-katello
The role I'm proposing replacing this with doesn't have a
foreman_installer_skip_installer or similar var, I think having a
foreman_installer role with with a don't actually run the installer is kind
of pointless.
One important and often used workflow for this is to be able to perform all
setup just prior to an installer run, then inspect or patch the system, and
then continue with the installer run. So while I can see how this wouldn't
make sense from a broad view, it is very handy for debugging.
… If we want to just install packages, maybe we should extend the
update_os_packages role or create a os_packages.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#516 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAR58_eCVpTkosci1CdK-X1mwSxif03Eks5sYhFOgaJpZM4O3owG>
.
--
Eric D. Helms
Red Hat Engineering
Ph.D. Student - North Carolina State University
|
On Tue, Aug 15, 2017 at 5:45 PM, sean797 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pipelines/pipeline_katello_32.yml
<#516 (comment)>:
> - "--disable-system-checks"
- - "--foreman-admin-password {{ foreman_installer_admin_password }}"
- foreman_installer_additional_packages:
- - katello
+ installer_scenarios_answers:
👍 I probably should rename that var...
Yea -- this goes a bit to "standards" differences as things get split out.
I'd recommend running the same Ansible linting that we run and double
checking over the best practices page (I am a big fan of the namespacing
all variables).
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#516 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAR589Ero5plEy8aWfATPsL3U8rGXrOIks5sYhFtgaJpZM4O3owG>
.
--
Eric D. Helms
Red Hat Engineering
Ph.D. Student - North Carolina State University
|
On Tue, Aug 15, 2017 at 5:50 PM, sean797 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pipelines/pipeline_katello_32.yml
<#516 (comment)>:
> - "--disable-system-checks"
- - "--foreman-admin-password {{ foreman_installer_admin_password }}"
- foreman_installer_additional_packages:
- - katello
+ installer_scenarios_answers:
+ foreman:
+ admin_password: changeme
This makes the role idempotent, if you modify foreman_installer_options
you have to run the installer to make sure its applied (everytime), wheras
this method allows you to manage the answer file and run the installer as a
handler (only if something changes). https://github.com/sean797/
ansible-role-foreman_installer/blob/master/templates/scenario-answers.
yaml.j2
That is an interesting approach. I like everything about it.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#516 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAR582igBJ7Jk0hoJIf7DSNNO_vdg5gOks5sYhKKgaJpZM4O3owG>
.
--
Eric D. Helms
Red Hat Engineering
Ph.D. Student - North Carolina State University
|
9f35aca
to
1daa1b4
Compare
Update, I think I addressed all your comments @ehelms. Thanks! |
I'd love to, but I've been very much focused on getting releases out and builds stable. 1.16 RC2 is long overdue. Even my own work with split installs has stalled. After that's settled down a bit I'll make time. |
@ehelms @ekohl I know you guys are busy but is their still appetite to replace the current role for an idempotent one? I find it quite frustrating using forklift with production boxes always runs the installer. Just to reiterate, I'm happy to move the role to theforeman GitHub Org or host it in Forklift. What might work is, hosting the role in Forklift and setting up a Jenkins job (can be trigger via a Github webhook) that pushes any changes to a dedicated repo for the Ansible role, that would allow it to be put on Galaxy and other Users to make use of it. We could obviously do the same thing for other roles. Though obviously setting up Jenkins could be somewhat time-consuming and a PITA. |
@sean797 you're right that I'm too busy to give proper attention to the various PRs. This large PR with some architectural implications is an easy victim. I think it'd be good to start a discourse thread about expanding the group of maintainers on this repository. I'd be happy to pass on maintenance to others while remaining available for questions. |
@sean797 This has sat for a while, and I think we are still on the fence about moving this out to it's own repository. |
Yea - lets close this |
This replaces foreman_installer role with https://github.com/sean797/ansible-role-foreman_installer (https://galaxy.ansible.com/sean797/foreman_installer/)
A couple of benefits this provides:
foreman_proxy_content
role)I've added it via git submodule as its also on Galaxy and I think it makes sense to not host it in forklift. I'm happy to define it in requirements.yml and/or move it to theforeman GitHub org.
I haven't changed pipelines/pipeline_katello_31.yml or pipelines/pipeline_katello_30.yml. Happy to do so, but I think they may be worth removing now, considering how old they are?