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

Install GitHub #17

Closed
wants to merge 5 commits into from
Closed

Conversation

brooklynbagel
Copy link

@brooklynbagel brooklynbagel commented Sep 29, 2017

Proposed extension to allow packages whose code is hosted on GitHub to be installed through the playbook. My attempt at the proposed enhancement #16

Caveat: I tested the PR locally using your included Vagrantfile and everything checks out on 14.04 and 16.04. However 12.04 seems to have some issues with downloading the packages using remotes::install_github.

@tersmitten
Copy link
Member

Can you have a look at the failing tests?

@brooklynbagel
Copy link
Author

Sure. Looking at the logs it appears that the installation of the Github package is completing properly but they are failing the idempotence test. I'll have to investigate further.

@@ -22,7 +22,7 @@
{% if item.repos is defined %}{{ item.repos }}{% endif %}
register: r_install_package
changed_when: "r_install_package.stdout_lines[-1] is defined and r_install_package.stdout_lines[-1] == 'changed'"
with_items: "{{ r_packages }}"
with_items: "{{ ( [{ 'name' : 'remotes' }] if r_packages_install_remotes == true else [] ) + r_packages }}"
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's possible to do this check based on the type property of r_packages?

Copy link
Author

Choose a reason for hiding this comment

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

Oh sorry, this might be ambiguous and you might decide I should recode this. All it's doing is prepending the remotes package from CRAN to the installation list if the user wants to have it. I did this way to avoid adding another task, but it results in some messiness with the packages list. I can fix this and split into another task if you think it's better stylistically.

@tersmitten
Copy link
Member

Fixed in #18

@tersmitten tersmitten closed this Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants