-
Notifications
You must be signed in to change notification settings - Fork 43
Apply some normalization on Gitlab API handling #630
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.
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
Conversation
Gitlab API response structure is not exactly the same than Github / Bitbucket. So we need to fix some of the logic here… Also avoid making useless API calls
Use direct API endpoint instead of making others calls to retrieve useless data…
|
|
||
| return $issue->showComments(); | ||
| $comments = $this->client->api('merge_requests')->showNotes($this->getCurrentProject()->id, $id); | ||
| return array_filter(array_map(function($note) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run the php-cs-fixer 👍
|
Ok, I've ran php-cs-fixer... |
| '<fg=white>Comment #%s</> by %s on %s', | ||
| $comment['id'], | ||
| $comment['user'], | ||
| empty($comment['created_at']) ? '' : $comment['created_at']->format('r') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have to do this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The created_at key can be null if there is no date in API data... Maybe it's too much, there is always a date... I'll remove it !
|
Allow to make only the API calls required to perform the requested action.
|
Hello @cordoval you were right 😄. There are other places where API calls can be optimized. I've commited the changes. I've also processed your different feedbacks. |
|
@shulard the build will push the new images on dockerhub for gush, after it builds and pushes successfully, can you docker pull and test these changes please? thanks so we have some validation. Thanks |
|
test with github and gitlab |
|
ok I'll try with docker 😄 |
|
Hello @cordoval Is the image already built ? I've pull from docker hub and I've got some errors against data manipulation (array to string, strstr with a first parameter as array...). On my local machine without docker, there is no problem with data structure... |
|
paste the errors and how you are running it, the image was built https://hub.docker.com/r/coder20078/gush/tags/ 20 hours ago, make sure you do docker pull coder20078/gush again, check how old is your images with docker images |
|
I've just made I ran it using the Example here : I'm using the master branch freshly updated. |
|
it works, the only problem are those warnings, which means that we are plugging an array instead somewhere. |
|
Yeah I know but with the same code base I don't understand why these warning are present on the docker version... |
|
because your error reporting is set to ignore these, also because really you don't have the same environment. |
I've found that there are simpler methods to retrieve data from Gitlab API library than the one used for the moment.
Also, Gitlab API response structure is not the same than Github, some keys must be updated.