fix: make scopes work with multiple scope-names #1360
Conversation
2ee4155 to
426b9b5
Compare
|
When I look at "Files changed" only one file is changed. Was that the intent? The second commit is a Merge commit which doesn't seem correct. It seems like this patch was started before gitlab/v4/objects/init.py was split up into multiple files. |
There was a problem hiding this comment.
Thanks a lot for this @klorenz I think some people have asked for this already and will be happy to see it fixed! I'll try to find related issues so we can close them if it fixes them.
When I look at "Files changed" only one file is changed. Was that the intent? The second commit is a Merge commit which doesn't seem correct.
It seems like this patch was started before gitlab/v4/objects/init.py was split up into multiple files.
I agree with @JohnVillalovos, looks like with your merge you lost the custom _types attributes. Could you re-add them in the split objects/ files? Also looks like you had an unrelated change in the initial commit, please submit that as a separate PR if you add it back ;)
Apart from John's comment see also my note on only overriding the right method.
And finally since I see from the other PR that you're comfortable with unit tests here could you add one to prove it fixes the issue? ;)
|
Hi, i struggled a bit with the linting checks you do and had to redo the
commits. I check today if everything is in there. Sorry for the trouble.
Regards, Kiwi
Nejc Habjan <notifications@github.com> schrieb am Sa., 6. März 2021, 18:43:
… ***@***.**** requested changes on this pull request.
Thanks a lot for this @klorenz <https://github.com/klorenz> I think some
people have asked for this already and will be happy to see it fixed! I'll
try to find related issues so we can close them if it fixes them.
When I look at "Files changed" only one file is changed. Was that the
intent? The second commit is a Merge commit which doesn't seem correct.
It seems like this patch was started before gitlab/v4/objects/*init*.py
was split up into multiple files.
I agree with @JohnVillalovos <https://github.com/JohnVillalovos>, looks
like with your merge you lost the custom _types attributes. Could you
re-add them in the split objects/ files? Also looks like you had an
unrelated change in the initial commit, please submit that as a separate PR
if you add it back ;)
Apart from John's comment see also my note on only overriding the right
method.
And finally since I see from the other PR that you're comfortable with
unit tests here could you add one to prove it fixes the issue? ;)
------------------------------
In gitlab/types.py
<#1360 (comment)>
:
> +class ScopesListAttribute(ListAttribute):
+ def set_from_cli(self, cli_value):
+ if not cli_value.strip():
+ self._value = []
+ else:
+ self._value = [item.strip() for item in cli_value.split(",")]
+
+ def get_for_api(self):
+ # Do not comma-split single value passed as string
+ if isinstance(self._value, str):
+ return [self._value]
+
+ return self._value
+
+
Since you are inheriting from ListAttribute you don't need to duplicate
methods. Just override the one you need to and maybe instead of copying the
previous comment, add one (or a docstring) about why it needs to be
different from the inherited class. Unless I missed a small difference in
set_from_cli then please correct me 😁
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1360 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE2WZAXOJ5D6YDNTFHHZDTTCJSVVANCNFSM4YWTNIWQ>
.
|
… _types to objects
Codecov Report
@@ Coverage Diff @@
## master #1360 +/- ##
==========================================
- Coverage 80.21% 80.03% -0.19%
==========================================
Files 73 73
Lines 3801 4022 +221
==========================================
+ Hits 3049 3219 +170
- Misses 752 803 +51
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -561,11 +561,10 @@ def test_create_project_with_values_from_file(gitlab_cli, tmpdir): | |||
| assert description in ret.stdout | |||
|
|
|||
|
|
|||
| def test_create_project_deploy_token(gitlab_cli, project): | |||
| def do_test_create_project_deploy_token(gitlab_cli, project, scopes, expected_scopes): | |||
There was a problem hiding this comment.
hint: you could keep a single test function here and parametrize the input/expected output with pytest:
@pytest.mark.parametrize(
"scopes,expected_scopes",
[
("read_registry", "['read_registry']"),
("read_registry,read_repository", "['read_repository', 'read_registry']"),
],
)
def test_create_project_deploy_token(gitlab_cli, project, scopes, expected_scopes):
...Then you don't need a custom helper function at all and it will be easy to add more edge cases later. See very similar example here:
python-gitlab/gitlab/tests/test_config.py
Lines 236 to 249 in af781c1
|
Hi @klorenz, coming back to this, I realized we've solved the same/similar problem in a more generic way in #1420 for all objects that need special list filters. I just ran your functional tests on the current master branch and they're passing, so I just wanted to check what your original issue was so that we can see if this is still needed for any objects. Could you let me know what CLI commands you were trying before that did not work properly? It seems like creating deploy tokens with scopes as a comma-delimited string ( |
|
Related PR which also fixes other issues we have with array variables passed to the GitLab API |
|
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
|
This PR was closed because it has been marked stale for 15 days with no activity. If this PR is still valid, please re-open. |
I have found out, that it was not possible to use multiple comma-separated scopes from command line as documented. These changes fixes that issue.