Skip to content

Use puppet_repositories from theforeman.operations collection#1289

Merged
evgeni merged 1 commit into
theforeman:masterfrom
ehelms:operations-collection
Jul 9, 2021
Merged

Use puppet_repositories from theforeman.operations collection#1289
evgeni merged 1 commit into
theforeman:masterfrom
ehelms:operations-collection

Conversation

@ehelms

@ehelms ehelms commented Feb 9, 2021

Copy link
Copy Markdown
Member

No description provided.

@ehelms ehelms force-pushed the operations-collection branch 2 times, most recently from 76ee84e to 46493e7 Compare February 10, 2021 02:46

@ekohl ekohl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should document how to get the ansible collection.

Another thing I dislike is that the requirements states pulp_installer is needed, but I won't need that for most operations. It means I'll pull in a lot more than I really need.

Now I also need to get stuff from galaxy. As someone who dislikes Ansible, this feels like a step back. I'm leaning to holding off on using any collections until there's a real benefit. I can see it in complex roles like where we now use hammer, but here it feels complex. It is literally more lines of code even if you don't take the added dependency into account.

@ehelms ehelms force-pushed the operations-collection branch 2 times, most recently from ca84b33 to dc24eba Compare February 12, 2021 01:58
@ehelms ehelms changed the title Use theforeman.operations.installer role for installation Use theforeman.operations.installer role for installation and repositories Feb 12, 2021
@evgeni

evgeni commented Feb 12, 2021

Copy link
Copy Markdown
Member

The idea of the operations collection was that we also dogfood it, and forklift seems like a good place for dogfooding :)

We could split the requirements files into Foreman and Pulp easily, would that help? Eric and I expected this to be a problem at some point due to different usecases.

@ehelms ehelms force-pushed the operations-collection branch 6 times, most recently from 9daf9e8 to 02abd95 Compare February 16, 2021 15:30
@ekohl

ekohl commented Feb 16, 2021

Copy link
Copy Markdown
Member

We could split the requirements files into Foreman and Pulp easily, would that help? Eric and I expected this to be a problem at some point due to different usecases.

I think that would be a good idea.

Comment thread ansible.cfg Outdated
Comment thread playbooks/luna_demo_environment.yml Outdated
Comment thread roles/foreman_staging_repositories/tasks/redhat.yml Outdated
Comment thread roles/foreman_staging_repositories/tasks/redhat.yml Outdated
Comment thread roles/foreman_server_repositories/meta/main.yml
@ehelms ehelms force-pushed the operations-collection branch 2 times, most recently from 9b126a2 to dce3e68 Compare February 17, 2021 12:59
@ehelms

ehelms commented Feb 17, 2021

Copy link
Copy Markdown
Member Author

We could split the requirements files into Foreman and Pulp easily, would that help? Eric and I expected this to be a problem at some point due to different usecases.

I am not seeing the value in splitting. Having one requirements.yaml means users running a single command, means our CI not having to track job-to-job which file to install from and when I checked locally this totaled 6 MB. If I extrapolate, and we started using FAM in some of the seeding roles, would that be another requirements.yaml file?

@ehelms ehelms force-pushed the operations-collection branch from dce3e68 to 033e520 Compare March 8, 2021 19:53

@ekohl ekohl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still think this is a bad execution.

First of all, we lose the ability to upgrade Puppet between releases. theforeman/foreman-operations-collection#20 is IMHO a blocker to merging it.

Then it unconditionally enables Katello repositories. That's also a big regression IMHO. Foreman must be able to install without Katello and the best way to test it is to not have the repository present. That it slows down tests is maybe a smaller issue but it also means that Foreman's pipeline can be red if the Katello repositories are offline.

Lastly I really dislike adding a dependency on ansible-galaxy, especially if it also pulls in the whole pulp installer with all its dependencies.

@ehelms

ehelms commented Mar 9, 2021

Copy link
Copy Markdown
Member Author

Then it unconditionally enables Katello repositories. That's also a big regression IMHO. Foreman must be able to install without Katello and the best way to test it is to not have the repository present. That it slows down tests is maybe a smaller issue but it also means that Foreman's pipeline can be red if the Katello repositories are offline.

I am missing something here. Katello repositories are only enabled when a variable is defined for that purpose: https://github.com/theforeman/foreman-operations-collection/blob/develop/roles/foreman_repositories/tasks/redhat.yml#L26

Lastly I really dislike adding a dependency on ansible-galaxy, especially if it also pulls in the whole pulp installer with all its dependencies.

You aren't really saying why you dislike having the dependency, or why it is bad for Forklift. Can you expand in a way that I can either make changes to address or explain more about it?

Comment thread requirements.yml Outdated
Comment thread roles/foreman_installer/tasks/install.yml Outdated
@ehelms

ehelms commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

I'll get around to updating this one once #1313 is in as I feel like I should get that set of changes in first and then make this one.

Comment thread roles/forklift_versions/library/forklift_versions.py Outdated
Comment thread roles/forklift_versions/library/forklift_versions.py

@ekohl ekohl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general I'm not a fan of Ansible and having to rely on more Ansible tools is something that I'm hesitant about.

For example, if I want to make a change to a role that's used and test it out, I know how to do it and easily send a PR. Instead of ansible-galaxy, can I point it to a git checkout to develop on?

Comment thread .gitignore Outdated
Comment thread pipelines/pipeline_katello_lbproxy.yml
Comment thread README.md
@ehelms

ehelms commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

For example, if I want to make a change to a role that's used and test it out, I know how to do it and easily send a PR. Instead of ansible-galaxy, can I point it to a git checkout to develop on?

You can do:

collections:
  - name: git+file:///home/user/path/to/repo/.git
    type: git

I could add this to the docs to give some lead in for developers to know how to do it.

@ehelms

ehelms commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

See #1313 for updated documentation regarding questions here. That PR is the first use of the collection that I want to get in and then come back and update this PR.

@ehelms ehelms force-pushed the operations-collection branch from 033e520 to 9ddc297 Compare June 18, 2021 12:24
@ehelms ehelms changed the title Use theforeman.operations.installer role for installation and repositories Use puppet_repositories from theforeman.operations collection Jun 18, 2021
@ehelms ehelms force-pushed the operations-collection branch from 9ddc297 to 264cbba Compare June 23, 2021 01:16
@evgeni

evgeni commented Jun 23, 2021

Copy link
Copy Markdown
Member

Thanks @ehelms!

I think there is one thing left: the old role used puppet_repositories_version to indicate which version to install, while the new one uses foreman_puppet_repositories_version, but nothing sets this new variable right now. We don't deploy anything but 6 right now, so it won't break today, but I guess soon we'll want to try 7.

We can either add a translation step that does foreman_puppet_repositories_version: "{{ puppet_repositories_version }}" or teach the forklift_versions role to set the right variable directly.

I prefer the later.

@ehelms ehelms force-pushed the operations-collection branch from 264cbba to be1699f Compare June 23, 2021 20:17
@ehelms

ehelms commented Jun 23, 2021

Copy link
Copy Markdown
Member Author

Done (I hope)

Comment thread playbooks/bats_pipeline_katello_nightly.yml Outdated
@evgeni

evgeni commented Jun 24, 2021

Copy link
Copy Markdown
Member

Ha!

git grep says you also want to edit:

'puppet_repositories_version' => version['puppet']

@evgeni

evgeni commented Jul 8, 2021

Copy link
Copy Markdown
Member

@ehelms would you mind finishing this up? the recent puppet.com key rotation broke nightly pipelines, and I expect this to fix it once and forever

@ehelms ehelms force-pushed the operations-collection branch from be1699f to f2d58a0 Compare July 8, 2021 14:42
@ehelms

ehelms commented Jul 8, 2021

Copy link
Copy Markdown
Member Author

Fixed up that one spot and dropped that playbook

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

probably a good idea, yeah, even if unrelated to the original change here.

@ehelms

ehelms commented Jul 8, 2021

Copy link
Copy Markdown
Member Author

There's probably a few things we ought to clean up, I am happy to do that separately from the intent of this change.

@evgeni evgeni merged commit 2ddb698 into theforeman:master Jul 9, 2021
@evgeni

evgeni commented Jul 9, 2021

Copy link
Copy Markdown
Member

I dropped the one file Ewoud pointed out in #1383

If you see more, please do the same :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants