Pulp repo

[sysadmind]

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

pulp_repo

ANSIBLE VERSION

ansible 2.0.1.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides

SUMMARY

This adds a module for working with pulp repos. http://www.pulpproject.org/. I added this under packaging/os because it seemed like the most relevant location for this module although it is not explicitly an os packaging module like others in this directory.

I created a class that lines up with a pulp server and added methods for getting and setting repo properties. I chose this design because it closely resembles the actual design of pulp and keeps the code logically separated into small methods.

ok: [L-Pulp101.localdomain]

[mscherer]

Since that method is parsing output ( in parse_repo_list), you need to make sure that the command is run with C locales. See https://github.com/ansible/ansible-modules-core/pull/3257/files for a example.

[resmo]

@mscherer wouldn't this be sufficient https://github.com/ansible/ansible-modules-extras/pull/1961/files#diff-6d7169a8a399575e4dfbcc7c8872ca83R232 ?

[mscherer]

Seems check mode return a different thing that non check mode, I think it would be cleaner to get the same dict returned.

[gregdek]

Thanks @sysadmind for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

[resmo]

default: null should be omitted if required: true

[gregdek]

Thanks @sysadmind for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

[jtyr]

This line should not be here.

[jtyr]

I would rather see an implementation which would call the Pulp REST API instead of the pulp-admin command. It makes the error handling and output parsing much simpler. Use the fetch_url() from the ansible.module_utils.urls for that. Also squash all the Git commits into one and do forced push to update your repo.

[gregdek]

@sysadmind A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

[jtyr]

I did some more tests and have found out this:

• The Pulp API runs via HTTPS only so you need to change the default pulp_host to https://127.0.0.1.
• It doesn't make much sense to put pulp_host into the example as the code runs locally on the Pulp server anyway.

Otherwise I see your point about the url_argument_spec() now. I can imagine that people will need to set certain parameters for all their pulp_repo tasks. It would be handy to have the possibility to predefine such set of parameters as a variable and then use it within all tasks without the need to copy them all every time (if you need to add a param, you can add it on one place instead of on 100 places). I'm doing that in the yum_repository and jenkins_plugin by using the params option. The usage for Pulp could be as follows:

---

- name: Pulp server repos
  hosts: pulp01
  vars:
    pulp_params:
      validate_certs: no
      url_username: admin
      url_password: admin
  tasks:
    - name: Create local repo
      pulp_repo:
        name: repo1
        params: "{{ pulp_params }}"
    - name: Create repo with feed
      pulp_repo:
        name: repo2
        feed_url: http://some.server.com/path/to/the/repo
        params: "{{ pulp_params }}"

The last but not least, the module should be called pulp_repository to follow the convention set by other modules (e.g. apt_repository, yum_repository, zypper_repository).

[jtyr]

I would rename this to feed to follow the convention given by pulp-admin with its --feed.

[gthieleb]

I have written my own python script to perform pulp repo creation, syncing and copying repos, which uses pulp-admin behind the scenes. But having an ansible module for pulp administration would be 👍 .

Do you think there would be a place in the module to support copy tasks? What about creating the repo config files for the client side?

[sysadmind]

@gthieleb I have a separate module that I have developed to copy content units between repositories. I think that it makes sense to keep units and repositories separate. It may also make sense to create a client side module for that task. Once the pulp_repo module is merged, I'll work on getting the other modules contributed upstream.

[dvanallen]

I would love to use this module and it looks like you guys have put a lot of work into it - anything I can do to help get this merged?

[jtyr]

@dvanallen You can test the module and comment with the magic word as explained here.

[sysadmind]

Yes @dvanallen, testing would be a HUGE help as many different users have different use cases for pulp and it's hard to test for them all. Please report back with any issues or the magic words as @jtyr links to above to help get this merged.

[Nosmoht]

shipit

If someone needs code to test it, here is how i did it on Vagrant. Comment the roles as you might not have them

---
- hosts: all
  become: true
  become_method: sudo
  vars:
    pulp_repos:
    - name: CentOS-7-x86_64
      feed: http://mirror.centos.org/centos/7/os/x86_64/
      relative_url: centos/7/base
    - name: CentOS-7-x86_64-updates
      feed: http://mirror.centos.org/centos/7/updates/x86_64/
      relative_url: centos/7/updates
    - name: WANdisco-CentOS-7-x86_64-git
      feed: http://opensource.wandisco.com/centos/7/git/x86_64/
      relative_url: wandisco/centos/7/git
    - name: ELRepo-7-x86_64-kernel
      feed: http://elrepo.org/linux/kernel/el7/x86_64/
      relative_url: elrepo/7/kernel
  roles:
  - role: epel
  - role: mongodb
  - role: qpid
  - role: apache
  - role: pulp
  post_tasks:
  - name: Ensure repo
    pulp_repo:
      name: '{{ item.name }}'
      repo_type: rpm
      feed: '{{ item.feed }}'
      relative_url: '{{ item.relative_url }}'
      url_username: admin
      url_password: admin
      validate_certs: false
      force_basic_auth: true
      state: '{{ item.state | default(omit) }}'
    with_items: '{{ pulp_repos }}'
    tags: pulp_repos

[coreaut]

@sysadmind you could check if the importer_ssl_ca_cert parameter is multiline. If not treat it as a file path to the certs?

edit: or use file lookup. Not sure what is more the "ansible way"? (Module supports paths or require file lookups)

[jtyr]

The lookup plugin is only for local files. You want to read content of remote file which ca be done only via extra task. I think that it would be nice to be able to define remote path in the importer_ssl_* vars. Just check if the content of the var is actually a path to a file and if yes then read the content.

[coreaut]

@jtyr only importer_ssl_ca_cert needs to be a string. The other importer_ssl_* vars already have to be file paths.
Thanks for the lookup hint 😄

[coreaut]

shipit

@sysadmind I fully switched to your module (replaced it with my own).
Everything worked and I encountered no bugs so far (I have 33 repos).

The only nuisance is the extra Task required for importer_ssl_ca_cert 😄.

[sysadmind]

@Nebelwolf I'll try and get around to updating it to work with your ssl ca certs. It's hard for me to test because I don't use that setting.

If I understand correctly, it can either be a file path or actual certificate content (multiline). Is that the case? If I can figure out those changes and get them pushed to a branch, would you be able to test? Or do you have a testing procedure (like with a pulp docker container) that I could use for testing this functionality so that I can confirm I fix it for you?

[coreaut]

@sysadmind
For example currently I have to do something like this:

  - shell: cat "{{pulp_redhat_feed_cacert}}"
    register: datacacert
    changed_when: false

  - name: create RedHat repos
    pulp_repo:
      url_username: admin
      url_password: "{{ pulp_admin_password }}"
      force_basic_auth: true
      name: "{{ item['name'] }}"
      feed: "{{ item['feed'] }}"
      pulp_host: {{ pulp_host_url }}
      relative_url: "{{ item['relative_url'] }}"
      importer_ssl_ca_cert: "{{ datacacert.stdout }}"
      importer_ssl_client_cert: "{{ pulp_redhat_feed_cert }}"
      importer_ssl_client_key: "{{ pulp_redhat_feed_key }}"
    with_items: "{{ pulp_repo_map.redhat }}"
    notify:
      - resync repos

You see the extra task for importer_ssl_ca_cert? I just meant it would be nice if I could just give it the file path as parameter. Now it needs the multiline var with the cert.

So you would need to read the cert contents from the file path in your module code and pass that to the api call.

Maybe you should accept only paths for importer_ssl_ca_cert? Would be more consistent with the other importer_ssl_* params?

I can test it when you change something. Sorry theres no docker container for this. You need an valid RedHat subscription to test it.

[sysadmind]

I think I have this working as you would expect @Nebelwolf and @jtyr. I was able to curl the api and see the certs/keys by both entering the content and specifying a path to a file. Can you please test to make sure it works on your systems?

[coreaut]

@sysadmind sorry for the delay. Yes it works now as expected thanks!

[sysadmind]

@Nebelwolf @Nosmoht @dvanallen @jtyr If there is no other feedback, I would appreciate your assistance in getting this merged. You can comment on this PR as stated in #1961 (comment) and that will mark this PR for merge by the maintainers. I appreciate your help and feedback in this PR.

[coreaut]

@sysadmind do I have to comment the "magic word" again?
my previous comment

shipit ⛵

[jtyr]

It would be better to have explicit imports like this:

from ansible.module_utils.basic import AnsibleModule
from ansible.module_utils.urls import fetch_url
from ansible.module_utils.urls import url_argument_spec

[jtyr]

This comment is not necessary as the bellow are not snippets (it used to be in <v2.x) but regular modules.

[jtyr]

The variable assignment is not necessary as you don't use the variable anywhere else. Just call the function itself:

try:
    self.get_repo_config_by_id(repo_id)

[jtyr]

There should be no space here.

[jtyr]

The same as above - no space here.

[ansibot]

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

[sysadmind]

Moving to core repo.