internal/gitserver: Improvement to RepoInfo method#39358
Conversation
For reasons unknown now, we were making an unnecesarily high number of requests since the `shards` map being created as a factor of number of repos. But it should only ever be the total number of shards of gitserver currently running.
I spent an unnecessarily high amount of time trying to "fix" this code for what I thought was not the optimal way of collecting and sending the API requests, until I had the aha moment and finally understood why we're doing it this way. As a result, added a comment to clarify this as well as renamed shard to repoInfoReq as it doesn't look like that's the right name here and confused me somewhat more when I was reading this code.
Currently, if the API server returns anythign but a 200 OK, we return the HTTP status, but swallow the HTTP body. This will contain important information that will be helpful to understand the error, which is not possible as of now.
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff ac88f10...0033d5c.
|
mollylogue
left a comment
There was a problem hiding this comment.
lgtm, thx for the detailed breakdown of the changes
mollylogue
left a comment
There was a problem hiding this comment.
Quick question: have you tested what this looks like in the error scenario? Does the error now show up in the UI in addition to the status code?
@mollylogue I noticed that this method doesn't have any tests at all. I'm holding off merging until I write some tests. Yeah, at the moment only the 500 status code is seen. |
|
@mollylogue Now added tests with what we expected to see in the UI. I haven't tested the UI itself but the test should be sufficient since we're not changing the core behaviour. |
@indradhanush I suggest spending a bit extra time and test the UI too, more thorough testing is something we can do better as a team, especially taking BB Projects sync project's lessons learned into consideration 😅 |
sashaostrikov
left a comment
There was a problem hiding this comment.
New changes LGTM!
Additional thanks for adding tests 🙏
I've added a note about testing the UI as well, for the sake of testing completeness :)
Also "reason" is better than "body".
|
@sashaostrikov That's a good callout! Thank you. The UI test was fruitful as well. Made a small tweak to the error message and error message handling. 😄 |
This PR makes two functional improvements to the RepoInfo client method and a semantic one.
The functional improvements are:
The semantic improvement is:
Note to reviewers
It's recommended to look at each commit sequentially to view the changes in an isolated way. This should make it easier to understand the changes in this PR vs looking at the entire diff at the same time.
The pain point with no response body for non 200 errors was first observer in this customer issue which also led to the other improvements as I was reading and understanding this code.
Test plan
UI should now look like this:
VS current behaviour without this PR:
Note for time travellers
Without strings.TrimSpace
With strings.TrimSpace