Add the ability to specify an install_dir to the gem module #38195

acatton · 2018-04-02T22:40:43Z

SUMMARY

Some system administrator want to separate some gem installation from the rest
of the system. A good example is gem install --install-dir /opt/fluentd fluentd.

This pull request allows users of the gem module in Ansible to specify a
directory where the gem is going to be installed.

In addition to the future, this adds partial tests for the module in question.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

module: packaging.languages.gem

ANSIBLE VERSION

ansible 2.6.0 (add-install-dir-to-gem 8e1c0ce4d6) last updated 2018/04/03 00:31:31 (GMT +200)
  config file = None
  configured module search path = ['<home>/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = <localdev>/ansible/lib/ansible
  executable location = <localdev>/ansible/.ansible/bin/ansible
  python version = 3.6.4 (default, Mar 13 2018, 18:16:01) [GCC 7.3.1 20180130 (Red Hat 7.3.1-2)]

ADDITIONAL INFORMATION

N/A

ansibot · 2018-04-02T23:22:21Z

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/modules/packaging/language/test_gem.py:35:5: E303 too many blank lines (2)

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/packaging/language/gem.py:0:0: E309 version_added for new option (install_dir) should be 2.6. Currently 0.0

click here for bot help

acatton · 2018-04-04T23:58:39Z

I'm bumping this, because I fixed all the unit test, but the integration tests are failing. And I can't understand how this is related to my fix... Are the integration tests flaky, or is it me?

samdoran · 2018-04-05T16:25:28Z

Looks like the test failures are due to external repositories that are inaccessible. I'll restart the tests.

acatton · 2018-04-05T21:46:59Z

It looks like it's working @samdoran . Thanks :) .

samdoran · 2018-04-06T13:27:44Z

.pytest_cache/v/cache/lastfailed

@@ -0,0 +1 @@
+{}


Can you remove the .pytest_cache entries from this commit please?

Of course I can.

This is embarrassing. I usually check what I commit, but not this time :( .

samdoran · 2018-04-06T13:45:41Z

lib/ansible/modules/packaging/language/gem.py

@@ -252,6 +272,8 @@ def main():
        module.fail_json(msg="Cannot specify version when state=latest")
    if module.params['gem_source'] and module.params['state'] == 'latest':
        module.fail_json(msg="Cannot maintain state=latest when installing from local source")
+    if module.params['user_install'] and module.params['install_dir']:
+        module.fail_json(msg="install_dir requires user_install=false")


Rather than adding an explicit check, you could just set user_install and install_dir as mutually exclusive.

Tell me if I'm wrong, but I'm not sure this is possible.

In order for this feature to work, the user MUST specify:

gem: user_install: no install_dir: /some/other/directory

The default of user_install being True.

Unless I'm mistaking, mutually_exclusive means that "the user CAN specify
one, but MUST NOT specify the other." However, here, the user MUST specify both.

If I'm right, there are two solutions:

Keep the current code (= the manual check)

Change the default of user_install to no, thus loosing backward compatibility.

Yes, you're correct. After more careful reading, I see what you're trying to accomplish. Changing the default would not be a good idea.

samdoran · 2018-05-04T16:47:42Z

Could you also add tests for state: absent when using a custom install_dir?

acatton · 2018-05-10T15:39:04Z

@samdoran you're right. Good catch. I've made the changes you recommended ;) .

ready_for_review

ansibot · 2018-05-15T00:33:11Z

The test ansible-test sanity --test no-tests-as-filters [explain] failed with 3 errors:

test/integration/targets/gem/tasks/main.yml:49:9: use `current_gems.stdout is search` instead of `current_gems.stdout | search`
test/integration/targets/gem/tasks/main.yml:65:9: use `not current_gems.stdout is search` instead of `not current_gems.stdout | search`
test/integration/targets/gem/tasks/main.yml:88:28: use `.path is search` instead of `.path | search`

click here for bot help

acatton · 2018-05-16T21:01:05Z

Thanks @samdoran for adding the tests, and updating the README 👍 .

Do you know what are the next steps to get that merge into devel? I'm not to familiar with Ansible's contribution process.

samdoran · 2018-05-21T13:56:02Z

Merged to devel for inclusion in 2.6. Thank you!

…38195) * Add the ability to specify an install_dir to the gem module * Add GEM_HOME when installing a non-global gem * Add tests for custom gem path * Fix sanity tests * Add changelog entry * Rebase and add tests for incorrect options Co-authored by: Antoine Catton <devel@antoine.catton.fr>

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Apr 2, 2018

acatton force-pushed the add-install-dir-to-gem branch 5 times, most recently from 16ec583 to 1ade944 Compare April 4, 2018 23:51

samdoran removed the needs_triage Needs a first human triage before being processed. label Apr 5, 2018

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 5, 2018

samdoran suggested changes Apr 6, 2018

View reviewed changes

ansibot removed the core_review In order to be merged, this PR must follow the core review workflow. label Apr 6, 2018

acatton and others added 4 commits May 15, 2018 00:17

Add the ability to specify an install_dir to the gem module

f5c5609

Add GEM_HOME when installing a non-global gem

3740cad

Add tests for custom gem path

21a963a

Fix sanity tests

f7b0399

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 15, 2018

samdoran approved these changes May 15, 2018

View reviewed changes

samdoran added 2 commits May 15, 2018 00:27

Add changelog entry

388608d

Rebase and add tests for incorrect options

6fe61ed

samdoran force-pushed the add-install-dir-to-gem branch from e3c175c to 6fe61ed Compare May 15, 2018 04:55

samdoran merged commit 39f9d3e into ansible:devel May 21, 2018

ansible locked and limited conversation to collaborators May 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to specify an install_dir to the gem module #38195

Add the ability to specify an install_dir to the gem module #38195

acatton commented Apr 2, 2018

ansibot commented Apr 2, 2018

acatton commented Apr 4, 2018

samdoran commented Apr 5, 2018

acatton commented Apr 5, 2018

samdoran Apr 6, 2018

acatton Apr 7, 2018

samdoran Apr 6, 2018

acatton Apr 7, 2018 •

edited

samdoran Apr 25, 2018

samdoran commented May 4, 2018

acatton commented May 10, 2018

ansibot commented May 15, 2018

acatton commented May 16, 2018 •

edited

samdoran commented May 21, 2018

Add the ability to specify an install_dir to the gem module #38195

Add the ability to specify an install_dir to the gem module #38195

Conversation

acatton commented Apr 2, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Apr 2, 2018

acatton commented Apr 4, 2018

samdoran commented Apr 5, 2018

acatton commented Apr 5, 2018

samdoran Apr 6, 2018

Choose a reason for hiding this comment

acatton Apr 7, 2018

Choose a reason for hiding this comment

samdoran Apr 6, 2018

Choose a reason for hiding this comment

acatton Apr 7, 2018 • edited

Choose a reason for hiding this comment

samdoran Apr 25, 2018

Choose a reason for hiding this comment

samdoran commented May 4, 2018

acatton commented May 10, 2018

ansibot commented May 15, 2018

acatton commented May 16, 2018 • edited

samdoran commented May 21, 2018

acatton Apr 7, 2018 •

edited

acatton commented May 16, 2018 •

edited