Add a way to test module PR's #322

stbenjam · 2016-10-11T19:22:05Z

Used like this for example:

qpid-localhost:
  box: centos7
  memory: 6144
  ansible:
    playbook: 'playbooks/katello.yml'
    group: 'server'
    variables:
        foreman_repositories_use_koji: True
        katello_repositories_use_koji: True
        foreman_installer_module_prs: "katello/candlepin/44,katello/katello/79,katello/capsule/96,katello/certs/65"
        puppet_repositories_version: 4

stbenjam · 2016-10-11T19:23:25Z

Still working on this, but opened this in case anyone has a comment about the general strategy.

ATM, passing the module hashes isn't working :-\ From vagrant -> ansible something is happening:

It works fine with just a straight playbook, but you get this using the example in the PR desc above.

TASK [katello : install module PR into installer directory] ********************
Tuesday 11 October 2016  14:18:46 -0400 (0:00:00.058)       0:22:03.578 ******* 
fatal: [qpid-localhost]: FAILED! => {"failed": true, "msg": "'unicode object' has no attribute 'owner'"}

Also, kafo is unhappy about dumping modules in the directory, I think it's something to do w/ parser caches.

ehelms · 2016-10-12T13:13:23Z

The code seems to be not correctly moving the passed hash to a python dict:

TASK [katello : debug] *********************************************************
Wednesday 12 October 2016  09:08:24 -0400 (0:00:00.093)       0:00:18.143 ***** 
ok: [qpid-localhost] => {
    "msg": "[{owner=>katello,"
}

msg: [{owner=>katello,

Notice the ruby syntax in there

stbenjam · 2016-10-12T13:38:26Z

Ah thanks, I'll try to figure it out. Seems like the Vagrant host_vars thing can't handle complex data types

ehelms · 2016-10-12T14:00:47Z

Looks like you might have to do this?

http://stackoverflow.com/questions/36841703/cannot-pass-nested-array-of-hashes-to-ansible-host-vars-in-vagrants-ansible-loc

stbenjam · 2016-10-12T14:01:57Z

Literally on the same stack overflow page :-) The vagrant ansible provisioner also just joins an array with " " so ruby dumps out it's own internal .to_s of the hash. I could fix it, but since the ini file can't have hashes, I'll just refactor them as strings and split on a slash. Blargh.

stbenjam · 2016-10-13T14:21:31Z

This version works. Using the box in the description. (Although, the install itself fails because of https://bugzilla.redhat.com/show_bug.cgi?id=1329327#c8)

ekohl

I think this is also a candidate for the docs. Other than that it looks good.

ekohl · 2016-10-17T14:15:37Z

playbooks/roles/katello/tasks/module_pr.yml

+      git clone https://github.com/{{ item.split("/")[0] }}/puppet-{{ item.split("/")[1] }} {{ item.split("/")[1] }} &&
+      cd {{ item.split("/")[1] }} &&
+      git fetch origin pull/{{ item.split("/")[2] }}/head:pr &&
+      git merge pr


I think git checkout pr is cleaner.

It is intentionally this way - the particular PR branch could be out of date and missing something important, and it reflects the reality of what will happen when the PR is actually merged on GitHub.

That's fair.

ehelms · 2016-10-17T14:37:18Z

One downside to this approach is it is tied into the katello module. For example, I was thinking this would be great to test #269 using theforeman/puppet-foreman#488 but cannot due to it not being stand-alone functionality.

ekohl · 2016-10-17T14:55:15Z

One downside to this approach is it is tied into the katello module.

Do you mean the puppet module or the ansible role?

stbenjam · 2016-10-17T15:00:43Z

The reason it's like this is because it has to be between the package install and installer execution.

We could make a foreman_installer role, and centralize all the installer calls including module pr's there (one way of solving #304).

katello.yml would look like:

- hosts: all
  become: true
  roles:
    - etc_hosts
    - epel_repositories
    - puppet_repositories
    - foreman_repositories
    - katello_repositories
    - katello (maybe call this katello_packages?)
    - { role: foreman_installer, scenario: "katello", <some other options> }

ehelms · 2016-10-17T15:27:07Z

@stbenjam sounds good, I realize its more roles but they kind of make things more composable

stbenjam · 2016-10-19T16:18:09Z

This is a pretty big refactor. Removed versioned playbooks, remove entire katello role. Thoughts?

The only capability that isn't present in this is katello/upgrade.yml but as far as I can see doesn't work at all ATM, it references old .yml files that don't exist anymore (when repos were part of katello role).

ekohl

I like this change. It unifies Foreman and Katello to the core components they share

ekohl · 2016-10-19T16:26:34Z

playbooks/foreman_platform.yml

@@ -0,0 +1,13 @@
+- hosts: all


Not sure why this is named foreman_platform when it installs Katello.

This is an existing playbook, I'm not adding it here, just refactoring it.

But, it is "foreman_platform" as it contains Foreman + many of the most common plugins.

Oh right, you renamed the specific foreman_platform_1.12 and heavily refactored it. Carry on :)

ekohl · 2016-10-19T16:26:53Z

playbooks/foreman_platform.yml

+      scenario: katello
+      foreman_installer_options_internal_use_only: "--disable-system-checks --enable-foreman-plugin-discovery --enable-foreman-plugin-bootdisk --enable-foreman-plugin-remote-execution --enable-foreman-plugin-hooks --enable-foreman-plugin-openscap --enable-foreman-proxy-plugin-openscap --enable-foreman-proxy-plugin-remote-execution-ssh"
+      foreman_installer_additional_packages:
+          - katello


Why is this needed? shouldn't the installer handle this or is the the katello scenario?

New foreman_installer role is generic, it only installs foreman-related packages. If you're using an alternate scenario, you need the package(s) that have it, I could make foreman_installer role know which packages are needed for which scenario but this seemed more generic

Ok, so the package katello provides the katello installer scenario. I'd have expected it to be something like foreman-installer-katello. And I'm fine with this more generic solution.

It is indeed named like that, but the katello package is meta-package that pulls in all the dependencies too. Makes the installer take less time since everything should already be on the box

ekohl · 2016-10-19T16:29:53Z

playbooks/foreman_platform.yml

+    - katello_repositories
+    - role: foreman_installer
+      scenario: katello
+      foreman_installer_options_internal_use_only: "--disable-system-checks --enable-foreman-plugin-discovery --enable-foreman-plugin-bootdisk --enable-foreman-plugin-remote-execution --enable-foreman-plugin-hooks --enable-foreman-plugin-openscap --enable-foreman-proxy-plugin-openscap --enable-foreman-proxy-plugin-remote-execution-ssh"


Can this be an array for some readability?

beav · 2016-10-19T20:33:07Z

this PR worked as advertised for me

Can the readme be updated to include the example from #322 (comment) ?

ehelms · 2016-10-19T20:35:37Z

playbooks/roles/foreman_installer/tasks/install.yml

+      --scenario "{{ foreman_installer_scenario }}"
+      --foreman-admin-password "{{ foreman_installer_admin_password }}"
+      {{ foreman_installer_options }}
+      {{ foreman_installer_options_internal_use_only | join(" ") }}


I think this looks great, so take tis question as a looking ahead. This seems ripe for an Ansible module. Would you agree or do you think that would be overkill? e.g.

- name: 'Run Installer' foreman_installer: scenario: foreman verbose: false options: > -- foreman-admin-password "changeme"

That would seem fine to do - looks cleaner

ehelms · 2016-10-19T20:37:02Z

playbooks/roles/foreman_installer/tasks/module_pr.yml

+      git clone https://github.com/{{ item.split("/")[0] }}/puppet-{{ item.split("/")[1] }} {{ item.split("/")[1] }} &&
+      cd {{ item.split("/")[1] }} &&
+      git fetch origin pull/{{ item.split("/")[2] }}/head:pr &&
+      git merge pr


Similar to above, thinking out loud about re-factoring this to a module.

ehelms · 2016-10-19T20:38:21Z

playbooks/katello.yml

+      foreman_installer_options_internal_use_only:
+          - "--disable-system-checks"
+      foreman_installer_additional_packages:
+          - katello


Soon we can hopefully avoid this via installer integration?

Ah yea I would think so, but still maybe needed, e.g. doesn't RHCI or some red hat cloud product bolt onto our installer with another package?

ehelms · 2016-10-19T20:47:19Z

I wanted to test this on a Content Smart Foreman Proxy as well and had to make these changes:

diff --git a/playbooks/capsule.yml b/playbooks/capsule.yml
index 196829d..3619bf0 100644
--- a/playbooks/capsule.yml
+++ b/playbooks/capsule.yml
@@ -11,3 +11,18 @@
     - foreman_repositories
     - katello_repositories
     - capsule
+    - role: foreman_installer
+      foreman_installer_scenario: capsule
+      foreman_installer_options_internal_use_only:
+          - '--disable-system-checks
+            --foreman-proxy-trusted-hosts "{{ server_fqdn.stdout }}"
+            --foreman-proxy-trusted-hosts "{{ ansible_nodename }}"
+            --foreman-proxy-foreman-base-url "https://{{ server_fqdn.stdout }}"
+            --foreman-proxy-register-in-foreman true
+            --foreman-proxy-oauth-consumer-key "{{ oauth_consumer_key.stdout }}"
+            --foreman-proxy-oauth-consumer-secret "{{ oauth_consumer_secret.stdout }}"
+            --capsule-certs-tar "{{ capsule_certs_tar }}"
+            --capsule-parent-fqdn "{{ server_fqdn.stdout }}"
+            --capsule-pulp-oauth-secret "{{ pulp_oauth_secret.stdout }}"'
+      foreman_installer_additional_packages:
+          - foreman-installer-katello
diff --git a/playbooks/katello.yml b/playbooks/katello.yml
index d4dcd3f..fc150ac 100644
--- a/playbooks/katello.yml
+++ b/playbooks/katello.yml
@@ -10,5 +10,6 @@
       foreman_installer_scenario: katello
       foreman_installer_options_internal_use_only:
           - "--disable-system-checks"
+          - "--foreman-admin-password {{ foreman_installer_admin_password }}"
       foreman_installer_additional_packages:
           - katello
diff --git a/playbooks/roles/capsule/tasks/install.yml b/playbooks/roles/capsule/tasks/install.yml
index 472cf21..03af7e7 100644
--- a/playbooks/roles/capsule/tasks/install.yml
+++ b/playbooks/roles/capsule/tasks/install.yml
@@ -60,17 +60,3 @@
   yum:
     name: foreman-installer-katello
   when: custom_install == False
-
-- name: 'Run Install'
-  shell: >
-    foreman-installer -v --scenario capsule --disable-system-checks
-    --foreman-proxy-trusted-hosts "{{ server_fqdn.stdout }}"
-    --foreman-proxy-trusted-hosts "{{ ansible_nodename }}"
-    --foreman-proxy-foreman-base-url "https://{{ server_fqdn.stdout }}"
-    --foreman-proxy-register-in-foreman true
-    --foreman-proxy-oauth-consumer-key "{{ oauth_consumer_key.stdout }}"
-    --foreman-proxy-oauth-consumer-secret "{{ oauth_consumer_secret.stdout }}"
-    --capsule-certs-tar "{{ capsule_certs_tar }}"
-    --capsule-parent-fqdn "{{ server_fqdn.stdout }}"
-    --capsule-pulp-oauth-secret "{{ pulp_oauth_secret.stdout }}"
-  when: custom_install == False
diff --git a/playbooks/roles/foreman_installer/tasks/install.yml b/playbooks/roles/foreman_installer/tasks/install.yml
index d2d670b..d8b79e8 100644
--- a/playbooks/roles/foreman_installer/tasks/install.yml
+++ b/playbooks/roles/foreman_installer/tasks/install.yml
@@ -3,7 +3,6 @@
   shell: >
       foreman-installer {{ (foreman_installer_verbose == True) | ternary("-v", "") }}
       --scenario "{{ foreman_installer_scenario }}"
-      --foreman-admin-password "{{ foreman_installer_admin_password }}"
       {{ foreman_installer_options }}
       {{ foreman_installer_options_internal_use_only | join(" ") }}
   when: not foreman_installer_skip_installer

stbenjam · 2016-10-19T20:49:17Z

@ehelms You can push directly to my branch (new feature)! https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

ehelms · 2016-10-19T20:54:14Z

@stbenjam That felt so wrong, and yet, so right.

ehelms · 2016-10-19T21:09:51Z

I think this will break the pipeline jobs we use for testing -- I can either work up the update (as I need to do some 3.2 testing) and add it to this PR or do it after the fact.

ekohl · 2016-10-19T21:52:22Z

playbooks/foreman.yml

@@ -4,4 +4,4 @@
    - epel_repositories


This playbook also needs become: true

ekohl · 2016-10-20T13:23:58Z

@ehelms I think fixing it after the fact is fine.

ehelms · 2016-10-20T18:31:33Z

playbooks/foreman_platform.yml

+          - "--enable-foreman-proxy-plugin-openscap"
+          - "--enable-foreman-proxy-plugin-remote-execution-ssh"
+      foreman_installer_additional_packages:
+          - katello


We should talk about syntax one of these days since we got a few styles creeping up. A quick google search didn't yield me a silver bullet style guide and ansible-lint doesn't check for that.

What other style would there be here?

ehelms · 2016-10-20T19:46:06Z

I've got some fixes for the pipeline's and working on an updated upgrade script I was using for testing as well. I think this is good to go in as is now though.

stbenjam · 2016-10-21T10:10:54Z

Awesome, thanks!

stbenjam force-pushed the module-prs branch from 235af02 to d03da7c Compare October 12, 2016 19:07

Add a way to test module PR's

bf8bbfd

stbenjam force-pushed the module-prs branch from d03da7c to bf8bbfd Compare October 12, 2016 21:10

stbenjam changed the title ~~[WIP] Add a way to test module PR's~~ Add a way to test module PR's Oct 13, 2016

komidore64 mentioned this pull request Oct 13, 2016

[Refactor] - Convert installer shell commands to an ansible module #304

Closed

5 tasks

ekohl requested changes Oct 17, 2016

View reviewed changes

Refactor everything

3957761

ekohl reviewed Oct 19, 2016

View reviewed changes

stbenjam added 2 commits October 19, 2016 15:14

Fixes

820feeb

Make foreman_installer_options a string

1663649

ekohl mentioned this pull request Oct 19, 2016

Refactor box loading to be more explicit #318

Merged

ekohl approved these changes Oct 19, 2016

View reviewed changes

ehelms reviewed Oct 19, 2016

View reviewed changes

Update Capsule role to use foreman_installer role

5785c14

ekohl reviewed Oct 19, 2016

View reviewed changes

Foreman become:true

ba1959a

ehelms reviewed Oct 20, 2016

View reviewed changes

ehelms approved these changes Oct 20, 2016

View reviewed changes

stbenjam merged commit aa8cce9 into theforeman:master Oct 21, 2016

stbenjam deleted the module-prs branch October 21, 2016 10:11

daviddavis pushed a commit to daviddavis/forklift that referenced this pull request Feb 21, 2017

Add a way to test module PR's (theforeman#322)

0451614

johnpmitsch pushed a commit to johnpmitsch/forklift that referenced this pull request Jun 27, 2019

Add a way to test module PR's (theforeman#322)

e0aaafb

Add a way to test module PR's #322

Add a way to test module PR's #322

Conversation

stbenjam commented Oct 11, 2016 • edited Loading

stbenjam commented Oct 11, 2016 • edited Loading

ehelms commented Oct 12, 2016

stbenjam commented Oct 12, 2016

ehelms commented Oct 12, 2016

stbenjam commented Oct 12, 2016

stbenjam commented Oct 13, 2016 • edited Loading

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stbenjam Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Oct 17, 2016

ekohl commented Oct 17, 2016

stbenjam commented Oct 17, 2016

ehelms commented Oct 17, 2016

stbenjam commented Oct 19, 2016

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beav commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Oct 19, 2016 • edited Loading

stbenjam commented Oct 19, 2016 • edited Loading

ehelms commented Oct 19, 2016

ehelms commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Oct 20, 2016

stbenjam commented Oct 21, 2016

stbenjam commented Oct 11, 2016 •

edited

Loading

stbenjam commented Oct 11, 2016 •

edited

Loading

stbenjam commented Oct 13, 2016 •

edited

Loading

stbenjam Oct 17, 2016 •

edited

Loading

ehelms commented Oct 19, 2016 •

edited

Loading

stbenjam commented Oct 19, 2016 •

edited

Loading