Details
- Reviewers
vlorentz anlambert - Group Reviewers
Reviewers - Maniphest Tasks
- T2987: Port gitlab lister to the new `swh.lister.pattern.Lister` API
- Commits
- rDLS1a19b2c74752: gitlab: Support authentication
Diff Detail
- Repository
- rDLS Listers
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 18718 Build 28973: Phabricator diff pipeline on jenkins Jenkins console · Jenkins Build 28972: arc lint + arc unit
Event Timeline
Build is green
Patch application report for D4940 (id=17583)
Rebasing onto 02871f16c9...
Current branch diff-target is up to date.
Changes applied before test
commit 83c669476fc11c2a88e1db6746de9e19767a3baf
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/158/ for more details.
| swh/lister/gitlab/lister.py | ||
|---|---|---|
| 117–119 | Listers workers are configured with warning log level in production so that's why we are using warning here. | |
Adapt according to review:
- keep constructor parameter super().init(...) call consistent
- decrease log-level to info (it will only be used for the main
gitlab.com instance but still might as well see it's indeed used)
| swh/lister/gitlab/lister.py | ||
|---|---|---|
| 117–119 | heh, i did not know or forgot :) either way, we should probably rethink that now that we are passing on all of them. as mentioned in my diff adaptation, it's not that much of a burden to have it log in info as most of the gitlab instance do not have credentials configured. We only have the one main instance gitlab.com so far. | |
Build is green
Patch application report for D4940 (id=17588)
Rebasing onto b6a69b2ed9...
First, rewinding head to replay your work on top of it... Applying: gitlab: Support authentication
Changes applied before test
commit 3d741ab3f4738f7bb246becc04ad920b242a41dd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/159/ for more details.
| swh/lister/gitlab/lister.py | ||
|---|---|---|
| 117–119 | I guess there must be a reason for using a warning level in production. I agree that info level is more adequate here and in other listers but knowing which credentials was used by the lister is of interest so the use of warning here. | |
| swh/lister/gitlab/lister.py | ||
|---|---|---|
| 117–119 | I recall (come to think of it) that they were way too verbose at one point in the past. | |
- Keep using logger.warning for credential log (aligned with other listers)
- Fix type (in another commit though)
Build has FAILED
Patch application report for D4940 (id=17591)
Rebasing onto b6a69b2ed9...
Current branch diff-target is up to date.
Changes applied before test
commit b66ffc6c441dba8d01325bf8c648ba667f14798c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 18:15:21 2021 +0100
gitlab: make url mandatory and add type
commit cc9cccdcc284738a3e20ea1a7a71f8c814c2ace6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/162/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/162/console
| swh/lister/gitlab/lister.py | ||
|---|---|---|
| 117–119 | Celery produces verbose INFO output only when a worker starts, the remaining logging is at the DEBUG level when a task is executing. I guess we could change the listers workers loglevel from warning to info in production and then adjust listers code when it is done. | |
Build has failed
I think the new swh.scheduler-0.9.2 broke the build, thus the build failure here.
possibly due to b93aa5b...
Build has FAILED
Patch application report for D4940 (id=17591)
Rebasing onto b6a69b2ed9...
Current branch diff-target is up to date.
Changes applied before test
commit b66ffc6c441dba8d01325bf8c648ba667f14798c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 18:15:21 2021 +0100
gitlab: make url mandatory and add type
commit cc9cccdcc284738a3e20ea1a7a71f8c814c2ace6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/163/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/163/console
Build has FAILED
Patch application report for D4940 (id=17593)
Rebasing onto ea8ecee541...
Current branch diff-target is up to date.
Changes applied before test
commit 0c9cbe833920bbc358df54de6a86cb79da4d47e6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 18:15:21 2021 +0100
gitlab: make url mandatory and add type
commit f49af7a3b03985eeef63639e78fab76bb8db6f6b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/164/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/164/console
Build is green
Patch application report for D4940 (id=17596)
Rebasing onto ea8ecee541...
Current branch diff-target is up to date.
Changes applied before test
commit f49af7a3b03985eeef63639e78fab76bb8db6f6b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/167/ for more details.
Build is green
Patch application report for D4940 (id=17600)
Rebasing onto bea9d6d147...
Current branch diff-target is up to date.
Changes applied before test
commit 544a38f109ac89fc6d1b5222ae5324539e350798
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/170/ for more details.
| swh/lister/gitlab/lister.py | ||
|---|---|---|
| 97 | yes, thanks, it got dealt with in another commit. | |
| swh/lister/gitlab/lister.py | ||
|---|---|---|
| 117–119 | sounds fine a plan to me. | |
I do not see any section regarding basic HTTP authentication for API requests in Gitlab API doc. Are you sure it is working ?
I guess using an API token here would be a better choice. A token can be easily generated through Gitlab user account Web UI.
Sorry I accepted the diff before seeing you used Basic Auth which I think is not working.
I do not see any section regarding basic HTTP authentication for API requests in Gitlab API doc. Are you sure it is working ?
Bravo, I did not check. I trusted the old code.
It's doing what was done in the earlier version [1]
I'll check now.
I guess using an API token here would be a better choice. A token can be easily generated through Gitlab user account Web UI.
Yes, and i recall that's what configured for the main gitlab instance.
Sorry I accepted the diff before seeing you used Basic Auth which I think is not working.
That happens ;)
I'll check if it's really working.
Bravo, I did not check. I trusted the old code.
It's doing what was done in the earlier version [1]
I'll check now.
lol, using the actual and solely token we have in production with the right query (so not what's coded here or in the old lister).
I got a beautful:
$ http --headers "https://gitlab.com/api/v4/projects?access_token=<token>" ... Www-Authenticate: Bearer realm="Protected by OAuth 2.0", error="invalid_token", error_description="Token is expired. You can either do re-authorization or token refresh." ... $ http --headers https://gitlab.com/api/v4/projects "Authorization:Bearer <token>" ... same ...
So that means we listed gitlab.com anonymously without any token so far.
So i'm wondering if that diff is needed at all now.
If i'm implementing this nonetheless, i'll go with the header (bearer) implementation.
Anyway, a huge thanks for the heads up.
If i'm implementing this nonetheless, i'll go with the header (bearer) implementation.
a new swhbot account got created for the main gitlab instance with a fresh new token.
Now on to implement this properly.
Build is green
Patch application report for D4940 (id=17616)
Rebasing onto bea9d6d147...
Current branch diff-target is up to date.
Changes applied before test
commit 1a19b2c74752a0a77539aaab1763340e8cef31a0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date: Mon Jan 25 15:32:49 2021 +0100
gitlab: Support authentication
Related to T2987See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/174/ for more details.