-
Notifications
You must be signed in to change notification settings - Fork 81
Adds pulp-content role for the pulp_content_app #64
Conversation
8afa0a4
to
05eb509
Compare
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.
In addition to the comments, could you please also add this to the user-sandbox.yml
playbook?
Looks great thanks!
roles/pulp3-content/README.md
Outdated
|
||
* `ansible_python_interpreter`: **Required**. Path to the Python interpreter. | ||
|
||
This role **is not tightly coupled** with the `pulp3-postgresql` role, but it does |
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.
s/pulp3-postgresql/pulp3/
|
||
[Service] | ||
EnvironmentFile=-/etc/default/pulp_workers | ||
EnvironmentFile=-/etc/default/pulp_workers_%i |
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.
Should we be sharing environment files with the workers?
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 don't think so. That is supposed to source this file's contents but that doesn't even make sense anymore for pulp3 anyway and should be removed (I think). Does this make sense to you?
roles/pulp3-content/tasks/main.yml
Outdated
systemd: | ||
daemon_reload: true | ||
name: pulp_content_app.service | ||
state: started |
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 should probably use the state from the pulp_content_app.started
.
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.
roles/pulp3-content/tasks/main.yml
Outdated
group: root | ||
mode: 0644 | ||
notify: Reload systemd and restart pulp content app | ||
become: 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.
both of these tasks should move into a block with become: true
since they need to be run as the same user in all cases.
platforms: | ||
- name: Fedora | ||
versions: | ||
- 28 |
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.
Since our tests use f27 and centos, I think we should include them too.
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 realize that you probably did it this way to mirror the workers role, which is also missing those platforms. Would you mind changing that as well while we are thinking about it?
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.
Fedora 27 is end of life, IMO we should disinclude it both here and with the tests, and use F28+
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.
Good call. Filed: https://pulp.plan.io/issues/4334
============= | ||
|
||
Install, configure, and set the state of pulp content app. | ||
|
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.
You should also document the pulp_content_app
variable even though it has a default.
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.
Instead of using and documenting the variable, I removed it
@@ -5,3 +5,4 @@ | |||
- pulp3-workers | |||
- pulp3-resource-manager | |||
- pulp3-webserver | |||
- pulp3-content |
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.
A+
@bmbouter sounds good. I'll take this issue as assigned and make the changes. |
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.
Looks good, makes sense--assuming latest round of tests passed, approved!
closes #4302
closes #4302
https://pulp.plan.io/issues/4302