Add deploy key for public repo #1301
Conversation
There was a problem hiding this comment.
@J-Thompson12 Can you elaborate on what is actually going on here? This seems like a band-aid over a bigger problem.
Passing a single bool value all the way down into the stack is a code smell for me.
Oh I absolutely agree @jpellizzari. This is definitely a patch over a bigger issue. There was some discussion with @palemtnrider about if we should always do a PR using go-git-providers and just merge the PR if auto-merge is true. The decision was that goes against the choice to use go-git whenever possible. The big issue we have is that we have this mixed functionality that will for some flags work for all git sources and other flag options will only work for supported git sources. |
pkg/services/auth/auth.go
Outdated
| visibility, err := a.gitProvider.GetRepoVisibility(ctx, repo) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting repo visibility: %w", err) | ||
| } | ||
|
|
||
| if *visibility == gitprovider.RepositoryVisibilityPublic { | ||
| if *visibility == gitprovider.RepositoryVisibilityPublic && !isInstall { |
There was a problem hiding this comment.
@J-Thompson12 Wouldn't removing this *visibility == gitprovider.RepositoryVisibilityPublic logic give us the result we are looking for?
We need a deploy key any time we are writing to a repo (regardless of it's visibility), correct?
There was a problem hiding this comment.
We only need the deploy on install. We dont need it when we are adding a repo because we dont make changes to those repos.
There was a problem hiding this comment.
This same code is used for add and install so i needed some way to say which one was using it. Since this is a temp fix till a decision is made i went the easy route with the variable.
There was a problem hiding this comment.
The statement that we don't need to deploy keys on app repos is temporarily valid since we'll need those when implementing automated image updates, which was in the wish list in the retro we had a couple of days ago.
So, probably removing the visibility validation seems to me as a better long-term solution.
J-Thompson12
force-pushed
the
deploy-key
branch
from
5bdd27e
to
399eb05
Compare
J-Thompson12
changed the title
|
The main reason for adding that check was that using an app public repo failed. I think a good solution for handling that could be adding a new With that in place we can check for it and just upload the deploy key when it's a configrepo. We endup with the same problem when in the future we decide to have the image updater feature, but at least with this new field the change is self contained and doesn't created special flows like |
|
@luizbafilho I agree with that solution but dont we end up with the same problem in the future when we want to implement automated image updates? |
we do, but as I stated:
That's not a random flag flowing around, it's part of the RepoURL itself. |
|
Sorry @luizbafilho what I was saying is that when we implement that feature wont we just start adding deploy keys to everything again. So we might as well just add them now. |
I think when that happens we'll have some sort of flag indicating that we have the image updater enabled for that repo and then we can use that to indicate we need to push the deploy key, but pushing it always creates a usability issue. |
|
@luizbafilho I agree with that solution. I think it makes sense for now and the future. |
J-Thompson12
force-pushed
the
deploy-key
branch
from
399eb05
to
882808f
Compare
| DryRun: installParams.DryRun, | ||
| // We need to set URL and ConfigRepo to the same value so a deploy key is created for public config repos | ||
| URL: installParams.ConfigRepo, | ||
| ConfigRepo: installParams.ConfigRepo, |
There was a problem hiding this comment.
I dont love having to set these two values but this is the only solution short of rewriting the entire GetGitClients functionality and all the places that use it.
There was a problem hiding this comment.
Just spoke with @luizbafilho and @J-Thompson12; approving this to unblock it for a release, but a follow up issue needs to be created and worked on ASAP to address the underlying issue and remove this tech debt.
Closes: #1278
What changed?
Revert changes made in #1249. I left in the changes for the acceptance tests because they were good tests to keep in.
Why?
How did you test it?
Manually and unit tests
Release notes
Documentation Changes