Only add deploy key when needed #1334
Conversation
J-Thompson12
added
bug
J-Thompson12
force-pushed
the
fix-deploy-key
branch
from
c1a7e6c
to
f1073ee
Compare
| case isExternalConfig: | ||
| providerUrl = params.ConfigRepo | ||
| default: | ||
| return nil, nil, nil |
There was a problem hiding this comment.
I am not really sure when we will ever hit this default option. I believe with the new directory structure we will always have a config-repo. I also deleted the two tests associated with this.
| configRepoClient, configRepoErr := authSvc.CreateGitClient(ctx, normalizedConfigRepo, targetName, params.Namespace, params.DryRun) | ||
| if configRepoErr != nil { | ||
| return nil, nil, configRepoErr | ||
| if *repoVisibility == gitprovider.RepositoryVisibilityPrivate { |
There was a problem hiding this comment.
A comment here about why we are doing this would be helpful. I try to leave comments wherever a non-obvious if statement appears.
There was a problem hiding this comment.
@J-Thompson12 Can you elaborate on your comment? Why only for a private repo? If I read the code, I can see what is happening; I don't need a comment to tell me that.
Write a message to the next person who is going to wonder why this is the way it is.
pkg/services/factory.go
Outdated
| return nil, nil, nil | ||
| // This is temporary. When config repo is always present in the config map this can be removed. | ||
| if params.ConfigRepo == "" && !params.IsHelmRepository { | ||
| params.ConfigRepo = params.URL |
There was a problem hiding this comment.
At this point we might want to get rid of the field ConfigRepo and just leave URL.
There was a problem hiding this comment.
That is only in the case someone has the same config repo as the app they are adding. So running gitops add app . would hit this.
J-Thompson12
force-pushed
the
fix-deploy-key
branch
from
50df5e7
to
8154051
Compare
| Namespace: params.Namespace, | ||
| } | ||
|
|
||
| _, err = authSvc.SetupDeployKey(ctx, secretName, targetName, normalizedUrl) |
There was a problem hiding this comment.
Just so I understand it, this was split out from the other func and is called discretely now?
There was a problem hiding this comment.
Yes thats exactly what is happening now.
| configRepoClient, configRepoErr := authSvc.CreateGitClient(ctx, normalizedConfigRepo, targetName, params.Namespace, params.DryRun) | ||
| if configRepoErr != nil { | ||
| return nil, nil, configRepoErr | ||
| if *repoVisibility == gitprovider.RepositoryVisibilityPrivate { |
There was a problem hiding this comment.
@J-Thompson12 Can you elaborate on your comment? Why only for a private repo? If I read the code, I can see what is happening; I don't need a comment to tell me that.
Write a message to the next person who is going to wonder why this is the way it is.
J-Thompson12
force-pushed
the
fix-deploy-key
branch
from
88aa33f
to
7483a2d
Compare
Closes: #1323
What changed?
Refactored deploy key code and removed quick fix. This adds a deploy key on install for a config repo and then on add it only adds a deploy key if the repo is private.
Why?
How did you test it?
Manually and current tests cover it
Release notes
Documentation Changes