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

Jump to bottom

Renaming roles, use underscores rather than dashes #287

Merged
merged 1 commit into from May 15, 2020
Merged

Renaming roles, use underscores rather than dashes #287

merged 1 commit into from May 15, 2020

Conversation

fao89
Copy link
Member

@fao89 fao89 commented May 5, 2020

mikedep333
mikedep333 requested changes May 5, 2020
View reviewed changes
Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

I know this is a draft, but I thought I'd provide feedback early.

IMHO, the biggest challenge is that we've been providing playbooks like example-use/playbook.yml as an example. Users are going to have the dashes in their custom playbooks.

The question is then do we want to try to help them with migrating?

I'm hoping in-repo symlinks are a solution with little to no side effects.

At least we don't have to worry about users having installed the roles from galaxy yet and being under folders like ~/.ansible/roles/ .

roles/pulp_workers/templates/pulpcore-worker@.service.j2 Outdated Show resolved Hide resolved
@fao89
Copy link
Member Author

fao89 commented May 5, 2020

I know this is a draft, but I thought I'd provide feedback early.

IMHO, the biggest challenge is that we've been providing playbooks like example-use/playbook.yml as an example. Users are going to have the dashes in their custom playbooks.

The question is then do we want to try to help them with migrating?

I'm hoping in-repo symlinks are a solution with little to no side effects.

At least we don't have to worry about users having installed the roles from galaxy yet and being under folders like ~/.ansible/roles/ .

I was looking at the issue about galaxy collection: https://pulp.plan.io/issues/6458
then I wanted to see how the renaming would affect the CI (that's why I put it in draft).
I can see CI looks good and I agree with you, we definitely need to add symlinks.

@fao89 fao89 force-pushed the 6663 branch 2 times, most recently from c30061f to e7d36ea Compare May 13, 2020 15:01
@fao89 fao89 marked this pull request as ready for review May 13, 2020 15:48
@fao89 fao89 requested a review from mikedep333 May 13, 2020 15:49
@fao89 fao89 force-pushed the 6663 branch 2 times, most recently from 8431b27 to cb029fa Compare May 13, 2020 16:11
@pulpbot
Copy link
Member

pulpbot commented May 15, 2020

Attached issue: https://pulp.plan.io/issues/6663

mikedep333
mikedep333 requested changes May 15, 2020
View reviewed changes
Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Asking @pulp/ansible-installer team for feedback on service renaming. See in-line comments.

roles/pulp_workers/templates/pulpcore-worker@.service.j2 Outdated Show resolved Hide resolved
roles/pulp_webserver/README.md Outdated Show resolved Hide resolved
roles/pulp/handlers/restart_all_pulp.yml Outdated Show resolved Hide resolved
@fao89 fao89 requested a review from mikedep333 May 15, 2020 17:04
@bmbouter
Copy link
Member

This all looks good to me. Thank you @fao89 !

mikedep333
mikedep333 approved these changes May 15, 2020
View reviewed changes
Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Thank you very much, and I am approving this now.

However, per the pulp_installers meeting, please make sure we have a good communication at release time about the change users have to make to their custom playbooks that reference the role names. We have yet to introduce any breaking changes to the installer since 3.0.0, not even variable names. It probably should be in the email?

dkliban
dkliban approved these changes May 15, 2020
View reviewed changes
Copy link
Member

@dkliban dkliban left a comment

Choose a reason for hiding this comment

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

Thank you! We'll make sure to call out these changes when we send out the email to pulp-list announcing the 3.4.0 pulpcore release.

@fao89 fao89 merged commit e85b537 into pulp:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkliban dkliban dkliban approved these changes

@mikedep333 mikedep333 mikedep333 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fao89 @pulpbot @bmbouter @dkliban @mikedep333