Regressions in gitlab-shell due to merging !214
(ping: @ash.mckenzie & @rspeicher)
I'm working on !222 (closed) to fix some regressions I caused (see #145 (closed)). Part of working on that fix was adding tests for bin/gitlab-shell
. There were no end-to-end tests for it before. When rebasing those tests on master
you can see that a lot is broken since v8.1.0.
To reproduce, check out my !222 (closed), then apply this patch:
diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index c6b28ce..78fdfe8 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -208,7 +208,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
begin
if defined?(@who)
@user = api.discover(@who)
- @gl_id = "user-#{@user['id']}" if @user
+ @gl_id = "user-#{@user['id']}"
else
@user = api.discover(@gl_id)
end
That undoes a fix for an existing bug in v8.1.0. I have no idea how to fix that bug on the new master, but let's leave that aside for now. Then when you run the tests I've added in spec/gitlab_shell_gitlab_shell_spec.rb
you can see that just one test fails (the test spotting that particular regression).
Now rebase on master with that change, for convenience here's rebased commit: https://gitlab.com/avar/gitlab-shell/commits/214-regression-demo
A few tests now fail, and this is all stuff that'll be broken due to !214 (merged), since it's testing the existing gitlab-shell behavior (there just weren't any tests before): https://gitlab.com/avar/gitlab-shell/pipelines/27465637
I'm also concerned (but have not confirmed or denied), that the new code is going to be making extra API calls in some cases that we weren't doing before. As noted in my 2e8b6702 ("Add support for SSH certificate authentication", 2018-06-14) I was bending over backwards with the "username" support to not have it be less efficient than the other endpoints, and one of the tricky bits to get that right is the part I had a merge conflict with above.