Percent-encode GraphQL repository.url

[felixfbecker]

I had narrowed down the problem to specific resolvers so here's my best shot to fix #15618.

What I am unsure about:

• Is this the right location for the utility function? I don't know our code organization in the backend well.
• Is the unit test good?
• Should there be some kind of integration test for the GraphQL fields? (I don't know how to add those)

[codecov]

Codecov Report

Merging #15745 (da00ee6) into main (8e33500) will decrease coverage by 0.00%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main   #15745      +/-   ##
==========================================
- Coverage   52.43%   52.43%   -0.01%     
==========================================
  Files        1635     1635              
  Lines       81727    81728       +1     
  Branches     7299     7155     -144     
==========================================
  Hits        42850    42850              
  Misses      35049    35049              
- Partials     3828     3829       +1     

Flag	Coverage Δ
go	52.38% <57.14%> (-0.01%)	⬇️
integration	28.33% <ø> (+0.01%)	⬆️
storybook	27.35% <ø> (ø)
typescript	52.54% <ø> (+<0.01%)	⬆️
unit	34.99% <ø> (ø)

Impacted Files	Coverage Δ
cmd/frontend/graphqlbackend/git_ref.go	9.80% <0.00%> (ø)
cmd/frontend/graphqlbackend/git_commit.go	32.90% <50.00%> (ø)
cmd/frontend/graphqlbackend/repository.go	18.90% <66.66%> (+0.34%)	⬆️
cmd/frontend/graphqlbackend/git_revision.go	21.73% <100.00%> (ø)
.../internal/codeintel/resolvers/graphql/locations.go	83.50% <0.00%> (-2.07%)	⬇️
cmd/repo-updater/repos/types.go	74.20% <0.00%> (-0.25%)	⬇️
client/web/src/settings/SettingsPage.tsx	76.47% <0.00%> (+11.76%)	⬆️

[unknwon]

Thanks, merging!

[felixfbecker]

@unknwon what do you think about an integration test for this to prevent regressions? The unit test tests that the utility function, but nothing tests that it's actually used, so I'm a bit worried about this issue popping up again in the future

[unknwon]

@unknwon what do you think about an integration test for this to prevent regressions? The unit test tests that the utility function, but nothing tests that it's actually used, so I'm a bit worried about this issue popping up again in the future

@felixfbecker SGTM. Would any of GitHub, Bitbucket Server or AWS Code Commit supports spaces in repository name? 🤔 If not, might be a bit troublesome to add.

[felixfbecker]

GitHub definitely not. The others I don't know, but I imagine not. But since Azure DevOps is supported by Sourcegraph through "Other", it would be good if the integration tests could test that, no?

[unknwon]

GitHub definitely not. The others I don't know, but I imagine not. But since Azure DevOps is supported by Sourcegraph through "Other", it would be good if the integration tests could test that, no?

@felixfbecker Agree with the point of having it tested.

Do you have a working example of the "Other" external service for Azure DevOps, doesn't seem we have docs for it.

[felixfbecker]

No, I had to figure it out myself too 🙈
You can check the config that worked on k8s.sgdev.org though.

[unknwon]

No, I had to figure it out myself too 🙈
You can check the config that worked on k8s.sgdev.org though.

The one on the k8s.sgdev.org does look like we would need exactly. Filed #15905.