chore: Deprecate ambiguous API method names and provide non-ambiguous method names

[jannfis]

This PR aims for getting rid of constant swagger.json modifications on codegen due to ambiguous API method names. Change is non-functional and fully backwards compatible. Deprecated API methods can then be removed in any future version, i.e. v2.

Checklist:

• Deprecation notice should be in release notes
• The title of the PR states what changed and the related issues number (used for the release note).
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Optional. My organization is added to the README.
• I've signed the CLA and my build is green (troubleshooting builds).

[codecov]

Codecov Report

Merging #3071 into master will increase coverage by 0.24%.
The diff coverage is 46.62%.

@@            Coverage Diff             @@
##           master    #3071      +/-   ##
==========================================
+ Coverage   38.41%   38.65%   +0.24%     
==========================================
  Files         168      168              
  Lines       18208    18297      +89     
  Branches      272      237      -35     
==========================================
+ Hits         6994     7073      +79     
- Misses      10341    10350       +9     
- Partials      873      874       +1

Impacted Files	Coverage Δ
ui/src/app/shared/models.ts	100% <ø> (ø)	⬆️
cmd/argocd/commands/cert.go	0% <0%> (ø)	⬆️
util/argo/audit_logger.go	0% <0%> (ø)	⬆️
cmd/argocd-util/main.go	4.83% <0%> (-0.23%)	⬇️
ui/src/app/shared/services/version-service.ts	80% <0%> (ø)	⬆️
controller/sync.go	69.84% <100%> (ø)	⬆️
util/helm/client.go	56.45% <100%> (+0.71%)	⬆️
cmd/argocd/commands/app.go	5.77% <100%> (+3.85%)	⬆️
pkg/apis/application/v1alpha1/types.go	64.15% <100%> (+0.79%)	⬆️
controller/appcontroller.go	43.54% <100%> (+0.07%)	⬆️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbf9bde...fe752d6. Read the comment docs.

[alexmt]

LGTM. I think it will be helpful to get consistent names in swagger. I don't know a better way of doing it.

@jessesuen do you have any objections?

[jannfis]

Will merge this PR without any objection received in the next 5 days (I think 30 days grace period should be more than sufficient).

Shall we add log warnings when deprecated methods are still being used? @jessesuen @alexmt?

[jessesuen]

Actually I don't agree with this. I feel we are unnecessarily moving to a flat namespacing, and eventually introducing API backwards incompatibility, for the sole purpose of making codegen slightly easier, which I think can be solved in a much easier way (e.g. rm -f swagger.json as part of make codegen).

Doing this will force us in the future to not allow method names to conflict in different gRPC services, even if the method name makes sense.

[jannfis]

Actually I don't agree with this. I feel we are unnecessarily moving to a flat namespacing, and eventually introducing API backwards incompatibility, for the sole purpose of making codegen slightly easier, which I think can be solved in a much easier way (e.g. rm -f swagger.json as part of make codegen).

I don't know if removing the swagger.json file before regenerating it would help solving the issue (update: I just checked, it already is removed during codegen), which is not about codegen failing or being complicated. The problem I have observed is, once you touch the API i.e. by adding a new method or modifying its parameters, swagger happily reorders/renames its "mixin" functions. This is causing unneccessary merge conflicts in existing PRs which also had API modifications and would otherwise be completely compatible and could be merged without the renaming of mixins by swagger. It can become overly frustrating to maintain such a PR in a mergeable state over time. Also, it's quite hard to keep track of actual changes in the swagger file, because the commit history is sprayed with otherwise unnecessary changes.

Also, we don't have to actually remove the old methods from the API, so we can keep backwards compatibility. Having them marked as deprecated will keep swagger from mixing them into spec, but they'll still be usable from a gRPC and REST client point of view.

Doing this will force us in the future to not allow method names to conflict in different gRPC services, even if the method name makes sense.

As for flat namespacing, yes, your point is valid. I'm not very happy with it either and would love a solution where we can keep non-unique method names and not have swagger mixing constantly messing up the resulting definition file.

[jannfis]

FYI, the following PR would probably solve our problems: grpc-ecosystem/grpc-gateway#1193

It'll be included with upcoming v1.14.4 version of grpc-gateway, I'm preparing a PR to update our code generators once v1.14.4 has been released (a first RC has been released already).

Closing this PR.

[johanbrandhorst]

https://github.com/grpc-ecosystem/grpc-gateway/releases/tag/v1.14.4 has been released.