Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: Yazdan Mohammadi <[email protected]>
  • Loading branch information
Oded-B and yzdann authored Jun 12, 2024
1 parent b35fdff commit 134d7c3
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 6 deletions.
2 changes: 1 addition & 1 deletion docs/installation.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ Configuration keys:
|`toggleCommitStatus`| Map of strings, allow (non-repo-admin) users to change the [Github commit status](https://docs.github.com/en/rest/commits/statuses) state(from failure to success and back). This can be used to continue promotion of a change that doesn't pass repo checks. the keys are strings commented in the PRs, values are [Github commit status context](https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status) to be overridden|
|`commentArgocdDiffonPR`| Uses ArgoCD API to calculate expected changes to k8s state and comment the resulting "diff" as comment in the PR. Requires ARGOCD_* environment variables, see below. |
|`autoMergeNoDiffPRs`| if true, Telefonistka will **merge** promotion PRs that are not expected to change the target clusters. Requires `commentArgocdDiffonPR` and possibly `autoApprovePromotionPrs`(depending on repo branch protection rules)|
|`usaSHALabelForArgoDicovery`| The Default method for discovering relevant ArgoCD applications(for a PR) rely on fetching all appliation in repo and checking the `argocd.argoproj.io/manifest-generate-paths` **annotation**, this might cause perfromance issue on repo with large number of ArgoCD applications. The alternative is to add SHA1 of the application path as a **label** and relay on ArgoCD server side filtering, label name is `telefonistka.io/component-path-sha1`.|
|`useSHALabelForArgoDicovery`| The default method for discovering relevant ArgoCD applications (for a PR) relies on fetching all applications in the repo and checking the `argocd.argoproj.io/manifest-generate-paths` **annotation**, this might cause a performance issue on a repo with a large number of ArgoCD applications. The alternative is to add SHA1 of the application path as a **label** and rely on ArgoCD server-side filtering, label name is `telefonistka.io/component-path-sha1`.|
<!-- markdownlint-enable MD033 -->

Example:
Expand Down
10 changes: 5 additions & 5 deletions internal/pkg/argocd/argocd.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func createArgoCdClient() (apiclient.Client, error) {
return clientset, nil
}

// This function is used to find an ArgoCD application by the SHA1 label of the component path its supposed to avoid performance issues with the "manifest-generate-paths" annotation method with requires pulling all ArgoCD applications(!) on every PR event.
// findArgocdAppBySHA1Label finds an ArgoCD application by the SHA1 label of the component path it's supposed to avoid performance issues with the "manifest-generate-paths" annotation method which requires pulling all ArgoCD applications(!) on every PR event.
// The SHA1 label is assumed to be populated by the ApplicationSet controller(or apps of apps or similar).
func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) {
// Calculate sha1 of component path to use in a label selector
Expand All @@ -183,15 +183,15 @@ func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo st
return &foundApps.Items[0], nil
}

// This is the default method to find an ArgoCD application by the manifest-generate-paths annotation.
// It assume the ArgoCD (optional) manifest-generate-paths annotation is set on all relevant apps.
// Notice that this method include a full list all ArgoCD applications in the repo, this could be a performance issue if there are many apps in the repo.
// findArgocdAppByManifestPathAnnotation is the default method to find an ArgoCD application by the manifest-generate-paths annotation.
// It assumes the ArgoCD (optional) manifest-generate-paths annotation is set on all relevant apps.
// Notice that this method includes a full list of all ArgoCD applications in the repo, this could be a performance issue if there are many apps in the repo.
func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) {
// argocd.argoproj.io/manifest-generate-paths
appQuery := application.ApplicationQuery{
Repo: &repo,
}
// AFAIKT I can't use standard grpc instrumentation here, since the argocd client abstracts to much (including the choice between Grpc and Grpc-web)
// AFAIKT I can't use standard grpc instrumentation here, since the argocd client abstracts too much (including the choice between Grpc and Grpc-web)
// I'll just manually log the time it takes to get the apps for now
getAppsStart := time.Now()
allRepoApps, err := appIf.List(ctx, &appQuery)
Expand Down

0 comments on commit 134d7c3

Please sign in to comment.