Add support of user under allow_to_push and allowed_to_merge in protect branches method #273

florentio · 2021-09-09T13:13:35Z

Continuation of #232

florentio · 2021-09-09T13:57:16Z

Hi @gdubicki @Pigueiras @barryib, this pull request aims to deal with allowed_to_push and allowed_to_merge in a following way:

branches:
  main:
    protected: true
    push_access_level: 30
    merge_access_level: 30
    unprotect_access_level: 40
    allowed_to_push:
      - access_level: 40
      - access_level: 30
      - user_id: 15
      - user: john
    allowed_to_merge:
      - access_level: 40
      - access_level: 30
      - user_id: 15
      - user: doe

The extra parameter allowed_to_push and allowed_to_merge in the config.yml file can contains either access_level or user or user_id. You can provide user_id if you know the id of user, otherwise use user attribute and assign the related gitlab username.
There is conversion of user to user_id in the process to transform that config to:

branches:
  main:
    protected: true
    push_access_level: 30
    merge_access_level: 30
    unprotect_access_level: 40
    allowed_to_push:
      - access_level: 40
      - access_level: 30
      - user_id: 15
      - user_id: 16
    allowed_to_merge:
      - access_level: 40
      - access_level: 30
      - user_id: 15
      - user_id: 17

To update the configuration, we can now use the gitlab api https://docs.gitlab.com/ee/api/protected_branches.html#example-with-user--group-level-access. The post data look like this:

 {
    "push_access_level": 30,
    "merge_access_level": 30,
    "unprotect_access_level": 40,
    "allowed_to_push": [
        {
            "access_level": 40
        },
        {
            "access_level": 30
        },
        {
            "user_id": 15
        },
        {
            "user_id": 16
        }
    ],
    "allowed_to_merge": [
        {
            "access_level": 30
        },
        {
            "access_level": 40
        },
        {
            "user_id": 15
        },
       {
            "user_id": 17
        }
    ]
}

and respect the excepted payload according to the documentation.

How do I deal with the configuration to be sure to update only if there is change to make?
I have separated the acces_level number from the user_id value for each push_access_levels and merge_access_levels attributes.

First when I read the exiting config of the branch from gitlab api in the get_only_branch_access_levels function inside branches.py, i retrieve 4 difference arrays :

current_push_access_levels an array of all acces_level in push_access_levels api response
current_merge_access_levels an array of all acces_level in merge_access_levels api response
current_push_access_user_ids an array of all user_id in push_access_levels api response
current_merge_access_user_ids an array of all user_id in merge_access_levels api response

Second, when I read the requested configuration (config.yml), i split it to have 4 arrays too :

requested_push_access_levels an array which contains push_access_level value and values of acces_level keys in allow_to_push
requested_merge_access_levels an array which contains merge_access_level value and values of acces_level keys in allow_to_merge
requested_push_access_user_ids an array of values of user_id/user keys in allow_to_push
requested_merge_access_user_ids an array of values of user_id/user keys in allow_to_merge

Then, we compare them to see if there is change before applying the new configuration

gdubicki · 2021-09-10T07:54:58Z

The approach you have taken looks good to me, @florentio . 😊

gitlabform/gitlab/branches.py

gdubicki · 2021-09-10T19:49:32Z

Please make the existing acceptance tests pass again and add new ones if needed for the coverage to not decrease, @florentio.

PS Don't worry about the codecov reporting a total drop of the coverage just after you push new commits. Its report gets updated as the acceptance tests finish, which takes a few minutes. :)

codecov · 2021-09-10T20:04:05Z

Codecov Report

Merging #273 (3023d51) into main (c0d2873) will decrease coverage by 53.91%.
The diff coverage is 3.33%.

@@             Coverage Diff             @@
##             main     #273       +/-   ##
===========================================
- Coverage   73.36%   19.45%   -53.92%     
===========================================
  Files          57       57               
  Lines        2099     2138       +39     
===========================================
- Hits         1540      416     -1124     
- Misses        559     1722     +1163

Impacted Files	Coverage Δ
gitlabform/processors/util/branch_protector.py	`0.00% <0.00%> (-72.98%)`	⬇️
gitlabform/gitlab/branches.py	`22.85% <10.52%> (-62.66%)`	⬇️
gitlabform/processors/group/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
gitlabform/processors/project/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
gitlabform/processors/project/badges_processor.py	`0.00% <0.00%> (-100.00%)`	⬇️
gitlabform/processors/project/project_processor.py	`0.00% <0.00%> (-100.00%)`	⬇️
...itlabform/processors/project/branches_processor.py	`0.00% <0.00%> (-100.00%)`	⬇️
...labform/processors/group/group_badges_processor.py	`0.00% <0.00%> (-100.00%)`	⬇️
...abform/processors/group/group_members_processor.py	`0.00% <0.00%> (-100.00%)`	⬇️
...abform/processors/project/deploy_keys_processor.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0d2873...3023d51. Read the comment docs.

florentio · 2021-09-13T07:23:49Z

Please make the existing acceptance tests pass again and add new ones if needed for the coverage to not decrease, @florentio.

PS Don't worry about the codecov reporting a total drop of the coverage just after you push new commits. Its report gets updated as the acceptance tests finish, which takes a few minutes. :)

Hi, Ok I will start working on acceptance tests

barryib · 2021-09-14T12:47:25Z

@gdubicki It sounds like your Test workflow has been manually disabled. Was it desired ?

gdubicki · 2021-09-14T17:34:34Z

Yes, @barryib . We have moved to running tests under this workflow: https://github.com/egnyte/gitlabform/actions/workflows/linter-and-tests.yml

I am not sure why the tests are not running in the PRs now. :(

gdubicki · 2021-09-14T17:36:24Z

Looks like the way to run the tests now is for me to get a branch from your fork and push it to egnyte/gitlabform.. :/

gdubicki · 2021-09-15T08:08:42Z

A few tests are flaky, but all of them are passing now. 🎊

We still need to increase the test coverage though as it has dropped a bit.
Please see the updated comment from Codecov, @florentio .

florentio · 2021-09-15T13:21:19Z

Hi @gdubicki . I'm working on the acceptance for the feature. At the end, it should resolve codecov issue.
But I've two important questions

The feature is related to Gitlab premium (https://docs.gitlab.com/ee/api/protected_branches.html#example-with-user--group-level-access. To test I had to activate a free trial of gitlab premium.
If I push that acceptance tests, It will fail unless we automate the installation of gitlab in CI to activate premium https://docs.gitlab.com/14.0/ee/user/admin_area/license.html. Do you have input for that?
I've to deal with users (create user and make it member of project related to the protected branch) to test the feature. Do you have any input about how I can reuse some part of the existing code to avoid code duplication ?

gdubicki · 2021-09-15T14:17:47Z

The feature is related to Gitlab premium (https://docs.gitlab.com/ee/api/protected_branches.html#example-with-user--group-level-access. To test I had to activate a free trial of gitlab premium.
If I push that acceptance tests, It will fail unless we automate the installation of gitlab in CI to activate premium https://docs.gitlab.com/14.0/ee/user/admin_area/license.html. Do you have input for that?

For now please share the result of such test that you have run locally as a screenshot and add the test commented out. I will deal with automating running those tests with GitHub Actions shortly.

I've to deal with users (create user and make it member of project related to the protected branch) to test the feature. Do you have any input about how I can reuse some part of the existing code to avoid code duplication ?

Sure! To create protected branches you can use a fixture like this one http://github.com/egnyte/gitlabform/blob/7151d65d51ad8b9e97954db9aaa7a158b1b24d3c/tests/acceptance/test_branches.py#L10-L37 , that you use by adding a parameter to your test method with the name matching the fixture function name, f.e. here http://github.com/egnyte/gitlabform/blob/7151d65d51ad8b9e97954db9aaa7a158b1b24d3c/tests/acceptance/test_branches.py#L53-L53 . Similar thing done for the users is here: http://github.com/egnyte/gitlabform/blob/7151d65d51ad8b9e97954db9aaa7a158b1b24d3c/tests/acceptance/test_members.py#L9-L28 and then http://github.com/egnyte/gitlabform/blob/7151d65d51ad8b9e97954db9aaa7a158b1b24d3c/tests/acceptance/test_members.py#L53-L53 .

florentio · 2021-09-16T15:39:50Z

Hi @gdubicki , bellow a full dump of my test output,
you can check those sections in the file, where the test I've implemented are passed.

tests/acceptance/test_branches.py::TestBranches::test__mixed_config_with_new_api
tests/acceptance/test_branches.py::TestBranches::test_protect_branch_with_old_api_next_update_with_new_api_and_userid_and_unprotect

test.log

amimas · 2021-09-18T04:14:31Z

Hi - not sure if I've missed it. Based on the description I think only users can be specified, but not a group. Is that correct?

gdubicki · 2021-09-18T05:34:22Z

Hi - not sure if I've missed it. Based on the description I think only users can be specified, but not a group. Is that correct?

In this PR as of now - yes.

Fixes gitlabform#227

…ct branches method

hoping that this will also trigger a GHA pipeline

…ranch

gdubicki · 2021-09-18T19:36:39Z

I have (re)added a way to run the tests using GitLab Premium in a Docker container and made it possible in GitHub Actions. Note: in GitHub Actions this will only work in this repo, NOT in its forks. This is GitHub Actions' secrets' limitation/security feature. We'll see how it goes.

But you can see that the tests for the new feature are passing when I pushed the commit to egnyte/gitlabform, here: https://github.com/egnyte/gitlabform/runs/3641552180?check_suite_focus=true#step:5:1581

Thank you for your contribution @florentio and @barryib ! Merging and releasing a new version with this feature in a moment.

barryib · 2021-09-20T11:57:56Z

Thank to @florentio and to you @gdubicki.

I have (re)added a way to run the tests using GitLab Premium in a Docker container and made it possible in GitHub Actions. Note: in GitHub Actions this will only work in this repo, NOT in its forks. This is GitHub Actions' secrets' limitation/security feature. We'll see how it goes.

This is the normal behavior and is what you want. The licence must remain secret in this repo only.

barryib reviewed Sep 10, 2021

View reviewed changes

gitlabform/gitlab/branches.py Outdated Show resolved Hide resolved

florentio force-pushed the fix227 branch from d5babe4 to f7e444e Compare September 10, 2021 09:00

florentio marked this pull request as ready for review September 10, 2021 15:34

barryib mentioned this pull request Sep 15, 2021

Run linter and tests workflow on every PR #274

Merged

florentio force-pushed the fix227 branch from 7151d65 to c847e70 Compare September 16, 2021 15:26

gdubicki mentioned this pull request Sep 17, 2021

Add possibility to add extra parameters in protect branches method #232

Closed

Pigueiras and others added 10 commits September 18, 2021 16:58

Add possibility to add extra parameters in protect branches method

c9a7a90

Fixes gitlabform#227

Add support of user under allow_to_push and allowed_to_merge in prote…

27a7f54

…ct branches method

fix duplication error for merge_access_levels

7c8f30a

remove duplication code in protect_branch

c9607a8

fix linting

d12e830

Fix typo

e1e5de9

hoping that this will also trigger a GHA pipeline

fix acceptance test

b653a41

fix acceptance for branch and file

a6d5b4a

fix assert comparaison in acceptance test

47ac951

fix assert comparaison in acceptance test

0b2ce15

gdubicki and others added 3 commits September 18, 2021 16:58

Try to trigger tests

03d402d

add acceptance test implementation for user id support in protected b…

d2ab103

…ranch

add comment in acceptance test related to the feature

faaef25

gdubicki force-pushed the fix227 branch from c847e70 to faaef25 Compare September 18, 2021 14:58

gdubicki added 4 commits September 18, 2021 17:06

Mark tests that require Gitlab license

a0ede91

Fix that path AGAIN

fa46b0f

This is either broken or not enabled, but it's not the point

a2f7c53

Document new features and testing options

3023d51

gdubicki merged commit 6805cfc into gitlabform:main Sep 18, 2021

gdubicki mentioned this pull request Sep 20, 2021

#276 Add wildcard support for skip_groups and skip_projects #277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of user under allow_to_push and allowed_to_merge in protect branches method #273

Add support of user under allow_to_push and allowed_to_merge in protect branches method #273

florentio commented Sep 9, 2021

florentio commented Sep 9, 2021 •

edited

gdubicki commented Sep 10, 2021

gdubicki commented Sep 10, 2021 •

edited

codecov bot commented Sep 10, 2021 •

edited

florentio commented Sep 13, 2021

barryib commented Sep 14, 2021

gdubicki commented Sep 14, 2021

gdubicki commented Sep 14, 2021

gdubicki commented Sep 15, 2021 •

edited

florentio commented Sep 15, 2021

gdubicki commented Sep 15, 2021

florentio commented Sep 16, 2021

amimas commented Sep 18, 2021

gdubicki commented Sep 18, 2021 via email •

edited

gdubicki commented Sep 18, 2021

barryib commented Sep 20, 2021

Add support of user under allow_to_push and allowed_to_merge in protect branches method #273

Add support of user under allow_to_push and allowed_to_merge in protect branches method #273

Conversation

florentio commented Sep 9, 2021

florentio commented Sep 9, 2021 • edited

gdubicki commented Sep 10, 2021

gdubicki commented Sep 10, 2021 • edited

codecov bot commented Sep 10, 2021 • edited

Codecov Report

florentio commented Sep 13, 2021

barryib commented Sep 14, 2021

gdubicki commented Sep 14, 2021

gdubicki commented Sep 14, 2021

gdubicki commented Sep 15, 2021 • edited

florentio commented Sep 15, 2021

gdubicki commented Sep 15, 2021

florentio commented Sep 16, 2021

amimas commented Sep 18, 2021

gdubicki commented Sep 18, 2021 via email • edited

gdubicki commented Sep 18, 2021

barryib commented Sep 20, 2021

florentio commented Sep 9, 2021 •

edited

gdubicki commented Sep 10, 2021 •

edited

codecov bot commented Sep 10, 2021 •

edited

gdubicki commented Sep 15, 2021 •

edited

gdubicki commented Sep 18, 2021 via email •

edited