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

Refs #13658 - remove cycle from puppet graph #106

Merged
merged 1 commit into from
Feb 15, 2016

Conversation

beav
Copy link
Contributor

@beav beav commented Feb 13, 2016

The previous commit (4ecc339) added two cycles to the puppet dep graph, causing katello-installer to fail.

The offending line was:

require        => [Exec['foreman-rake-db:seed'], Class['::certs::pulp_client']],

Note that each of these requires individually will cause a cycle. The cycle for just foreman-rake-db:seed is http://i.imgur.com/eA8rWc6.png and the cycle for ::certs::pulp_client is http://i.imgur.com/EgeI2X2.png.

Removal of these lines makes katello-installer succeed.

@ehelms
Copy link
Member

ehelms commented Feb 13, 2016

However, this removes the guarantee that this is run at the correct time given this requires the certs exist and db is migrated. The better fix might be to remove the pulp client certs from the giant dependency block in init.pp.

@beav
Copy link
Contributor Author

beav commented Feb 13, 2016

@ehelms katello::config was needed to seed the db, but the db seeding was needed for katello::config. I broke out the pulp client config so it can happen after the db seeding.

Additionally I implemented your other suggestion for the other cycle.

🚳 🚳 🚳

ignore_missing => false,
require => [Exec['foreman-rake-db:seed'], Class['::certs::pulp_client']],
}
class { '::certs::pulp_client': }
Copy link
Member

Choose a reason for hiding this comment

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

Might as well move this into the pulp_client_config class, and I might consider just using the namespace, e.g.

config/pulp_client.pp
katello::config::pulp_client

The previous commit
(theforeman@4ecc339)
added two cycles to the puppet dep graph, causing `katello-installer` to fail.

The offending line was:

```
require        => [Exec['foreman-rake-db:seed'], Class['::certs::pulp_client']],
```

Note that each of these `require`s individually will cause a cycle. The cycle
for just `foreman-rake-db:seed` is http://i.imgur.com/eA8rWc6.png and the cycle
for `::certs::pulp_client` is http://i.imgur.com/EgeI2X2.png.

This refactoring avoids the cycles while still maintaining deps.
@beav
Copy link
Contributor Author

beav commented Feb 13, 2016

latest commit has a cycle, fixing

@beav
Copy link
Contributor Author

beav commented Feb 13, 2016

@ehelms I lied, sorry. Current PR passes my test and is ready for review.

@ehelms
Copy link
Member

ehelms commented Feb 13, 2016

I think this looks good, @ekohl mind weighing in on the design change here?

@ehelms
Copy link
Member

ehelms commented Feb 15, 2016

I will porbably need to test this in the installer, the current implementation results in:

[ WARN 2016-02-15 10:32:03 verbose]  Scope(Class[Katello::Config]): Could not look up qualified variable '::certs::pulp_client::client_cert'; class ::certs::pulp_client has not been evaluated
[ WARN 2016-02-15 10:32:03 verbose]  Scope(Class[Katello::Config]): Could not look up qualified variable '::certs::pulp_client::client_key'; class ::certs::pulp_client has not been evaluated

@ekohl
Copy link
Member

ekohl commented Feb 15, 2016

I can't judge if the pulp_client ~> pulp_parent relation is needed (and in turn pulp and qpid), but it sounds like it shouldn't be.

@ehelms
Copy link
Member

ehelms commented Feb 15, 2016

This passed my installer testing - thanks @beav !

ehelms added a commit that referenced this pull request Feb 15, 2016
Refs #13658 - remove cycle from puppet graph
@ehelms ehelms merged commit c652d94 into theforeman:master Feb 15, 2016
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