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.

Sign up for GitHub

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

Jump to bottom

gitlab_project_members: Added group sharing and unsharing logic for issue #8658 #8676

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gitlab_project_members: Added group sharing and unsharing logic for issue #8658 #8676

wants to merge 1 commit into from

Conversation

Acarnesecchi
Copy link

SUMMARY

Modifies the ansible_project_members module to add the possibility to share and unshare projects with one or more groups. This is supported both by the API and the python-gitlab library.

Fix #8658

ISSUE TYPE

gitlab_project_members.py

ADDITIONAL INFORMATION

@ansibullbot
Copy link
Collaborator

@Acarnesecchi this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html new_contributor Help guide this first time contributor labels Jul 25, 2024
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jul 25, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch labels Jul 25, 2024
@felixfontein felixfontein changed the title Added group sharing and unsharing logic for issue #8658 gitlab_project_members: Added group sharing and unsharing logic for issue #8658 Jul 25, 2024
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI merge_commit This PR contains at least one merge commit. Please resolve! labels Jul 25, 2024
@Acarnesecchi
Copy link
Author

@felixfontein I am trying to reach out to someone on libera.chat IRC but apparently I am not allowed to send messages in #ansible-devel channel. What can I do?
I have already rebased my fork so it's up-to-date but all commits are squashed.

@felixfontein
Copy link
Collaborator

@Acarnesecchi please ignore the IRC part of the bot's message, you don't need to ask any core team member there (this is a collection, not ansible-core; and also the problem here isn't that a core review is needed).

The main problem here is that your commit contains a lot of changes that don't belong to this PR and shouldn't be in there. If you check out the Files tab of this PR you can see what I mean: https://github.com/ansible-collections/community.general/pull/8676/files

@Acarnesecchi Acarnesecchi reopened this Jul 25, 2024
@Acarnesecchi
Copy link
Author

Acarnesecchi commented Jul 25, 2024 •
edited
Loading

@felixfontein my fork is clean now, please, let me know if anything else is needed. Thx!

@ansibullbot
Copy link
Collaborator

cc @Lunik @Shaps @lgatellier @marwatk @metanovii @nejch @scodeman @sh0shin @suukit @waheedi @zanssa
click here for bot help

@ansibullbot ansibullbot added plugins plugin (any type) and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jul 25, 2024
felixfontein
felixfontein reviewed Jul 27, 2024
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Can you please add a changelog fragment? Thanks.

plugins/modules/gitlab_project_members.py Outdated Show resolved Hide resolved
plugins/modules/gitlab_project_members.py Show resolved Hide resolved
plugins/modules/gitlab_project_members.py Outdated Show resolved Hide resolved
plugins/modules/gitlab_project_members.py Outdated Show resolved Hide resolved
plugins/modules/gitlab_project_members.py Outdated Show resolved Hide resolved
plugins/modules/gitlab_project_members.py
type: list
elements: dict
suboptions:
name:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use two different fields for ID and name, instead of having to figure out whether a string is an ID or a name.

Copy link
Author

Choose a reason for hiding this comment

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

All the other modules that use the group variable work with both ID and name. I would leave it as it is for consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know whether it's this way in the other modules for backwards compatibility, or because it is the best solution. I hope that some of the GitLab module maintainers can weight in here.

plugins/modules/gitlab_project_members.py Outdated Show resolved Hide resolved
plugins/modules/gitlab_project_members.py Outdated
elif module.params['gitlab_group'] is not None:
gitlab_groups = module.params['gitlab_group']
groups = project.get_groups_in_a_project(gitlab_project_id)
if groups:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This condition looks wrong to me. If module.params['gitlab_group'] is not None but groups happens to be empty, the below cases are handled, and eventually the module exits with "Nothing to do, please give at least one user or group or set purge_users true.". That doesn't sound right to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This problem is still there. Just because the module argument is not None doesn't mean the list is not empty.

plugins/modules/gitlab_project_members.py Outdated
changed_data = []

for gitlab_user in gitlab_users_access:
gitlab_user_id = project.get_user_id(gitlab_user['name'])
if members:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This condition also looks suspicious.

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jul 27, 2024
@Acarnesecchi Acarnesecchi reopened this Jul 27, 2024
@ansibullbot ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jul 27, 2024
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jul 27, 2024
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 29, 2024
@Acarnesecchi Acarnesecchi reopened this Jul 29, 2024
@ansibullbot ansibullbot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 29, 2024
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein Awaiting requested review from felixfontein

Assignees
No one assigned
Labels
backport-9 Automatically create a backport for the stable-9 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: Extend gitlab_project_members to support Group invitations
3 participants
@Acarnesecchi @ansibullbot @felixfontein