Community: GitLab community pinboard

[dagwieers]

We could collectively benefit from forming a Working Group related to GitLab integration. We have quite some contributors and users that are interested in improving this integration.

So this issue is a call to potential interested parties (earlier and existing contributors to Ansible). The benefits of having a Working Group is that members of the Working Group can:

• test, review and add shipits to existing GitLab PRs
• work collectively on a GitLab roadmap
• provide a single-point-of-contact for interested GitLab contributors
• collaborate on a GitLab-specific Wiki
• have a $team_gitlab in BOTMETA.yml for auto-notifying team members (ansible/ansible#51176)
• implement auto-labeling GitLab issues and PRs (ansible/ansible#51176)

Since we don't currently have GitLab infrastructure for automated testing, collaborating on modules like this is important for the quality of the modules.

[dagwieers]

cc @marwatk @dj-wasabi @SamyCoenen @waheedi @morph027 @drzraf @Draiken @pilou- @smutel @msimonin @miguelaferreira

[Shaps]

Interested in this

[dagwieers]

If you want to actively lead or are interested to be part of this Working Group, add your name to the Wiki page ! If we have a large enough group, we can start our own GitLab Working Group.

I started labeling all the GitLab related issues and PRs so we can more easily track them:

• https://github.com/ansible/ansible/pulls?q=is:pr+is:open+label:gitlab (8 open PRs)
• https://github.com/ansible/ansible/issues?q=is:issue+is:open+label:gitlab (9 open issues)

PS If you no longer want to receive any messages from this pinboard, feel free to unsubscribe from this issue ticket.

[marwatk]

Added my name to the Wiki. I think one of the first things we need to do is figure out an architecture for how modules should look so we can incrementally move in that direction.

For example, some of the older modules use pyapi-gitlab which hasn't been updated in a year and breaks on newer versions of GitLab. There appear to be multiple efforts to switch (1, 2), but I think a higher level discussion is warranted.

I'm not sure we want to rewrite everything, but having an approach to use for new modules would be ideal and we can work from there to update old ones over time.

[dagwieers]

@marwatk I will add you to $team_gitlab as well ! https://github.com/ansible/ansible/commit/152d7b674d908c9bcb9c9a8ee28c83bf9f255a95

[drzraf]

The more GitLab-Ansible modules there are (which is great), the more a GitLab API abstraction will makes sense. standardizing parameters looks like starting an API (especially with URL, connection, credentials, ... parameters) I believe that's how pyapi-gitlab and python-gitlab started.

In terms of API there is:

• python-gitlab
• pyapi-gitlab (used by gitlab_user, gitlab_project, gitlab_group) and unsuitable

I suggested two [pending] PR last year:

• https://github.com/ansible/ansible/pull/40053 (GitLab services), where I explained my choice of python-gitlab
• https://github.com/ansible/ansible/pull/39991 (GitLab webhooks), which is still under review after one year... but in the meantime PR https://github.com/ansible/ansible/pull/40096 (webhooks too, but not relying upon python-gitlab) was provided and merged under 3 days... and which... is going to be rewritten in the multi-hundred lines PR https://github.com/ansible/ansible/pull/51141

Note that at that time the only issue I could see with python-gitlab was the lack of Python 2.6 support.
Other than that, dependencies are requests and six

If will look at OpenStack modules, they seem to take DRY and NIH seriously in that they:

1. share a common codebase
2. and are based on openstacklib

About support, I think a module that stop working with GitLab.com API should be either dropped, obsoleted or (better) adapted, even if it's still working with self-hosted instances (which can be running at any version of the API).

[drzraf]

About tests, the issue affect all GItLab modules relying on 3rd party services (and there are a lot). That's something core GitLab team should offer some pointers and advises about the road to take.

[Lunik]

I'm currently working on refactoring previous gitlab modules to unify the api call, workflow and input parameters ansible/ansible#51141 .

[morph027]

Chiming in here....reading every tiny bit, but right now i'm quite busy at work. No room for coding, but of course for helping and discussion.

[dj-wasabi]

Hi all,

I'm not sure if I can help/assist with the coding, but can help with discussion/helping/reviewing/testing of modules.

I'll add my name to the list in the wiki.

Kind regards,
Werner

[dj-wasabi]

I forgot to ask and I should look around first but, does Ansible have certain standards where a PR should met with? Not that I say shipit and where someone else would say that certain things should be changed?

[dagwieers]

@dj-wasabi The process is explained briefly at: https://docs.ansible.com/ansible/devel/community/development_process.html

• So Shippable CI will test some basic stuff which we automated, these are formal rules and @ansibot will report any issues. The contributor should fix these before community review.
• A formal review by the community is about:
  
  
  
  • Does the module offer all functionality expected by users/possible by technology, and if incomplete, would the interface (parameters/usage) prevent the module to be extended consistently ?
  
  
  • Does it implement check-mode, diff mode ? Is it idempotent ?
  
  
  • Does the module have integration tests ? If not, we should urge to write integration tests and review those integration tests to see if they perform all possible tests. These typically test for idempotency and proper return values
  
  
  • Is the implementation sound? Obvious cornercases, potential problems, improvements to code?
  
  

We expect a shipit when the reviewer did his best at reviewing the code and testing it for real. If you cannot test it because you lack the infrastructure, please say so when approving the PR, but do not add a shipit! Because people that look for merging PRs most likely can't test the module either, and rely on this information to merge. Modules should be verified and tested by the community before being accepted.

[dagwieers]

Here are two checklists for contributors, but are equally useful for reviewers:

• Developing modules checklist
• Developing modules best practices

[dj-wasabi]

Hi @dagwieers

Thank you for your answer(s)! One small thing though, there are no technical "requirements" as in it should pass any pep standard? (I think I read/heard it somewhere that this was coming to Ansible, but not sure if this is already active.)

Thanks in advance.
Kind regards,

Werner

[dagwieers]

@dj-wasabi PEP8 testing is enforced by Shippable, like a lot of other sanity tests (and reported back by @ansibot). So if it passes Shippable tests, you know it's fine from that perspective.

[Lunik]

I have initiated a wiki page Gitlab-Module-Guidelines

[Lunik]

Hi,
Does someone have the time to review this PR ansible/ansible#51141 ?

[dagwieers]

@Lunik I updated the guidelines to make parameter validate_certs the standard name.
Also I wonder if it wouldn't be better to make https://gitlab.com/ the default server_url ? Rather than making it required (and no default).

[Lunik]

Also I wonder if it wouldn't be better to make https://gitlab.com/ the default server_url ? Rather than making it required (and no default).

If it was Github, it seems legit. But for Gitlab, I don't know 🤔
I'm not against so I can add that to my PR.

[Shaps]

@dagwieers I tend to use gitlab 90% from self-hosted and the rest is hosted on gitlab.com, I'm likely weird, if you think it's what people use most I'm +1 to it.

[dagwieers]

@Shaps The only reason for me to propose it as the default is because it's the only real option if we wanted a default. And for people using gitlab.com it wouldn't require this parameter. But if you think it is confusing for (the majority? of) users as they may be using the wrong target and would not understand why things are failing, I would be inclined to NOT do this and explicitly ask for this.

Leave it up to you guys to decide...

[dagwieers]

The amount of support here is great. And in the meantime both @Shaps and @Lunik stepped forward to lead the Gitlab Working Group. My role will be to help out where I can to advance the Gitlab integration.

We are still learning how these communities can help improve contributors and users. I started to write down how I envision the different roles in Working Groups. Feedback and ideas are always welcome.

There is room for more people taking up the Lead role, so if you think you have some extra cycles to spare to help grow this community, let me know.

The Ansible v2.8 freeze is nearing and we have about a month to get as much as possible reviewed and fixed. Feel free to reach out to @Shaps, @Lunik, @gundalow and me if you need any help. Let's make the upcoming release the best one for Gitlab ever!

[Lunik]

I think ansible/ansible#44055 and ansible/ansible#44069 is not related to gitlab

[Lunik]

python-gitlab module is not compatible with python 2.6
As a result, all units tests will fail with something like that:

/usr/local/lib/python2.6/dist-packages/_pytest/python.py:395: in _importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
/usr/local/lib/python2.6/dist-packages/py/_path/local.py:662: in pyimport
    __import__(modname)
/usr/local/lib/python2.6/dist-packages/_pytest/assertion/rewrite.py:212: in load_module
    py.builtin.exec_(co, mod.__dict__)
test/units/modules/source_control/test_gitlab_runner.py:5: in <module>
    from gitlab.v4.objects import Runner
/usr/local/lib/python2.6/dist-packages/gitlab/v4/objects.py:22: in <module>
    from gitlab.base import *  # noqa
E     File "/usr/local/lib/python2.6/dist-packages/gitlab/base.py", line 242
E       for self_attr, parent_attr in self._from_parent_attrs.items()}
E         ^
E   SyntaxError: invalid syntax

[drzraf]

That's right, though, citing https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#minimum-version-of-python-3-x-and-python-2-x :

For Python 2, the default is for modules to run on at least Python 2.6. This allows users with older distributions that are stuck on Python 2.6 to manage their machines. Modules are allowed to drop support for Python 2.6 when one of their dependent libraries requires a higher version of Python. This is not an invitation to add unnecessary dependent libraries in order to force your module to be usable only with a newer version of Python; instead it is an acknowledgment that some libraries (for instance, boto3 and docker-py) will only function with a newer version of Python.

[dagwieers]

python-gitlab module is not compatible with python 2.6

For this specific issue it is easy to make it work though.
Was Python 2.6 support removed, or is this just an oversight we are allowed to fix ?

https://github.com/python-gitlab/python-gitlab/pull/32#issuecomment-316473903

[drzraf]

As commented https://github.com/ansible/community/issues/431#issuecomment-456599678, you may refer to https://github.com/python-gitlab/python-gitlab/issues/275#issuecomment-308983621
AFAIR there are a couple of (handful) python 2.7-only constructs used here and there in the library.

Anyway:

• I'm tend to think a large portion of gitlab_* modules users run it on the client (localhost playbook or delegate: localhost)
• I doubt those provisioning GitLab instances are doing such from a Python 2.6-only machine (I can hardly envision a use-case where no better solution exist)

[Lunik]

Was Python 2.6 support removed, or is this just an oversight we are allowed to fix ?

From the python-gitlab Wiki it seems that they do.

python-gitlab is compatible with Python 2.7 and 3.4+.

And since Ansible dev guide allow it, well...

Modules are allowed to drop support for Python 2.6 when one of their dependent libraries requires a higher version of Python.

[dagwieers]

@Lunik I know this, but since it's easy to add Python 2.6 support we could take both paths (if upstream is willing).

[Lunik]

@dagwieers yep sorry, I misunderstood what you sayed. I will dig into the python-gitlab API to see what we can do to support python 2.6

[dagwieers]

@Lunik Nope, the python-gitlab developers don't want to see any Python 2.6 support even when I offered to make the required changes. At least that is a very clear answer and closes this path.

Obviously this would only affect people from running these modules remotely (on a Gitlab server or any SSH proxy). If we make it clear from the requirements: and the notes:, and potentially from a future Gitlab documentation in the Ansible docs, I guess we're fine.

Update: I will close these comments as resolved.

[Lunik]

As mentioned in https://github.com/ansible/ansible/pull/39991#issuecomment-459264051, should we add alias on this module to rename it gitlab_hook instead of gitlab_hooks ?

FMI, link to the documentation https://docs.ansible.com/ansible/latest/dev_guide/module_lifecycle.html#changing-a-module-name

[dagwieers]

@Lunik It was never released in an Ansible release, so I would not bother with aliases, but simple rename the module. Also, I need to know what the relation is with gitlab_webhook and if they do the same thing (or should be merged) we need to make sure that we are shipping in Ansible v2.8 whatever is best in the long-term for our users.

[Lunik]

@dagwieers

It was never released in an Ansible release, so I would not bother with aliases, but simple rename the module.

gitlab_hooks module is here since Ansible 2.6 so I don't understand what you mean.

gitlab_hooks and gitlab_webhook have exactly the same purpose with different approach.

Pros/Cons gitlab_webhook

| Pros | Cons |
|:------|:------|
| Better handling of hook event in module parameters. Using only on option of type list | No backward compatibilities with gitlab_hooks |
| Allow to delete hook by is ID | Could be confusing. Creating the hook with url deleting it with hook_id or url. |
|| Usage of deprecated parameters since ansible/ansible#51141 (server_url, login_user, ...) |
| Usage of the python-gitlab library ||
|| Module parameter solo diverts user from best practice. It allow to have multiple hooks pointing to the same url. There is no need since hook can have multiple events |
|| Need to be reviewed with new best practice like missing_required_lib and docs_fragements |

[dagwieers]

gitlab_hooks module is here since Ansible 2.6 so I don't understand what you mean.

I was mistaken, not sure how I got this wrong. Yes, renaming is probably best.

[Shaps]

Re- #40053
According to @sivel's comment the gitlab_service module violates the module guidelines and would not be accepted. With this in mind how do we want to proceed?

[drzraf]

I'd think it's premature to take this sole comment (which is one interpretation of the guidelines in the context of a specific MR), as a kind of legal-precedent. #40053 is far from closed :)

[gundalow]

https://github.com/ansible/ansible/pull/53897 Gitlab runner inventory plugin

[Lunik]

New issues have popped up about contains call to Display.deprecated or AnsibleModule.deprecated
How should we fix them ? Is there a porting guide somewhere in the documentation ?

ansible/ansible#55313
ansible/ansible#55314
ansible/ansible#55315
ansible/ansible#55316
ansible/ansible#55317

[dagwieers]

@Lunik Either fixing the issue (i.e. float 2.10 is converted to 2.1) or removing the statements because the deprecation window has ended (i.e. feature is removed in v2.9).

[Lunik]

All issues have been fixed and back-ported to Ansible 2.8

[Lunik]

Can someone make an additional review of ansible/ansible#56574 ?

[waheedi]

Hi @Lunik i went through #56574 and ran the tests successfully on Ubuntu 18.04 machine. Have not really dived into the code though.

[Lunik]

Can someone make an additional review on ansible/ansible#60425 ?

[Lunik]

Can someone make an additional review on ansible/ansible#60425 ?

This issue begin to be more and more critical with the time, people start complaining #63448

[Lunik]

Can we discuss about the relevance of actual unit tests ?
They are just mock API responses from GitLab API.
Can we just replace them by the integration tests we already have ?

[Shaps]

@Lunik I think the only reason we still have not moved to integration tests is because we don't really have a platform where to test the stuff on. I had started doing some work to implement this with ansible-test but life got in the way and haven't had a chance to finalise it.
I believe a good way forward would be to have the integration tests anyway, and keep them disabled in CI, at least we can still manually run them when required.

[Shaps]

@Lunik All the gitlab modules have now been migrated to the community.general collection, we should make an effort on updating them so they are "collection-compatible". Do you think you're going to have some time to spend on it? I think I can spare some time otherwise :)

[Lunik]

@Shaps What are you thinking about ? With the migration there should be a backward compatibility ?
Are you proposing to create our own collection for GitLab modules ?