-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat: Add autocomplete for repo Revisions (#4645) #4713
Conversation
0b7c8c1
to
fbfacf2
Compare
- Introduces api/v1/repositories/{repo}/refs which returns branches and tags - Add new RevisionFormField component to Create and Edit Application pages Signed-off-by: Tim Etchells <tetchell@redhat.com>
fbfacf2
to
7596701
Compare
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.
Awesome change! Added few minor commentss
util/git/git_test.go
Outdated
assert.NotContains(t, lsResult.Branches, testTag) | ||
assert.NotContains(t, lsResult.Tags, testBranch) | ||
|
||
//fmt.Printf("BRANCHES: %s\n", strings.Join(lsResult.Branches, ", ")) |
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 remove commented code.
load={async (src: any): Promise<string[]> => { | ||
if (src.repoURL) { | ||
try { | ||
const revisionsRes = await services.repos.revisions(src.repoURL); |
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.
I think it would be easy to read if we don't use async. E.g.
return services.repos.revisions(src.repoURL).
then(revisionsRes =>[ 'HEAD' ].concat(revisionsRes.branches).concat(revisionsRes.tags) ).
catch(() => []);
What do you think?
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.
Sure, doesn't matter to me
reposerver/repository/repository.go
Outdated
s.metricsServer.IncPendingRepoRequest(q.Repo.Repo) | ||
defer s.metricsServer.DecPendingRepoRequest(q.Repo.Repo) | ||
|
||
// TODO is locking necessary here? We are not modifying the repository, just running an ls-remote |
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.
Locking is not required ( it is necessary only if we are touching local repo files). Please remove comment
reposerver/repository/repository.go
Outdated
return nil, err | ||
} | ||
|
||
// TODO check cache |
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 ls-remote
is used if every other repo server operation without caching. In fact it is used to find cache key. So I think caching is not required here and we can remove this comment.
Signed-off-by: Tim Etchells <tetchell@redhat.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4713 +/- ##
==========================================
+ Coverage 41.10% 41.13% +0.03%
==========================================
Files 127 127
Lines 17367 17548 +181
==========================================
+ Hits 7139 7219 +80
- Misses 9202 9298 +96
- Partials 1026 1031 +5 ☔ View full report in Codecov by Sentry. |
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.
LGTM
👍
…zation-generators * 'master' of github.com:argoproj/argo-cd: fix: RevisionFormField component crashes in 'refs' API returns no tags (argoproj#4735) docs: add Opensurvey to USERS.md (argoproj#4727) docs: correct parameters usage in CLI (argoproj#4725) fix: Repo-server has silent unmarshalling errors leading to empty applications (argoproj#4423) (argoproj#4708) fix: inject artificial delay between sync waves to better support health assessments (argoproj#4715) fix: exclude files listed under exclusions (argoproj#4686) feat: support resource actions on CRDs that use status subresources (argoproj#4690) feat: Add autocomplete for repo Revisions (argoproj#4645) (argoproj#4713) fix: webhook don't refresh apps pointing to HEAD (argoproj#4717) feat: Add support for ExecProvider cluster auth (argoproj#4600) (argoproj#4710) fix: adding helm values file in New App (argoproj#4635) docs: Instructions on `make verify-kube-connect` step when using k3d (argoproj#4687) feat: Annotation based app paths detection in webhooks (argoproj#4699) fix: adding commonAnnotations for Kustomize (argoproj#4613)
Signed-off-by: Tim Etchells tetchell@redhat.com
Checklist:
Resolves #4645
See issue for screenshots