-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fedora changes: #966
Fedora changes: #966
Conversation
@@ -7,7 +7,7 @@ Requirements | |||
------------ | |||
|
|||
This role assumes it's being deployed on a RHEL/Fedora based host with package | |||
named 'etcd' available via yum. | |||
named 'etcd' available via yum for dnf (conditionally). |
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.
nitpicky, but s/for/or/
@maxamillion thanks for submitting this! outside of my mostly nitpicky comments, 👍 from me. On a side note, I can't wait until we can declare ansible 2.0 a requirement, because being able to just call the package task instead of conditionalizing all of the package installs would be nice. @sdodson heads up this has potential to impact your containerization PR. |
Thanks for the heads up. Other than being surprised that this is necessary it all looks good. LGTM |
@detiber Thanks for the review. I'll fix up the things you mentioned. Also massive +1 for Ansible 2.0 ... super excited about it. |
@sdodson - Unfortunately yes, at least for now. In Ansible 2.0 this won't be necessary (it'll still work, just won't be necessary) because they are adding a generic |
@twiest ptal |
I rebased on master, fixed merge conflicts and fixed the repo handler to work with both yum and dnf |
Crap ... I need to fix some things, the merge went bad |
@maxamillion thanks again for working on this! |
Alright, rebased, fixed merge conflicts and squashed commits. |
- name: refresh yum cache | ||
command: yum clean all | ||
|
||
- name: refresh dnf cache | ||
command: yum clean all |
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.
Shouldn't this be dnf clean all
?
After that we get clarification or change based on my note it is a 👍 from me. Though eventually I would like to see a package install role where we can have the |
@wshearn consider this temporary until we can require ansible 2.0, then we can use the new and improved package module. |
- ansible bootstrap playbook for Fedora 23+ - add conditionals to handle yum vs dnf - add Fedora OpenShift COPR - update BYO host README for repo configs and fedora bootstrap Fix typo in etcd README, remove unnecessary parens in openshift_node main.yml rebase on master, update package cache refresh handler for yum vs dnf Fix typo in etcd README, remove unnecessary parens in openshift_node main.yml
ok, 👍 from me. |
@wshearn if you are good with this, can you merge? |
NOTE:
OpenShift doesn't currently work because of some bugs in Fedora 23 land[0][1][2] - thanks to eparis for the help -, but this pull request does allow a successful origin deployment on Fedora. Also note that I plan to remove the special case Fedora COPR for Openshift once OpenShift is successfully brought into the Fedora official repositories via the Fedora OpenShift Change Proposal. I will open another pull request to fix that up when the time comes.