feat: new repo resolver logic to fetch info from a gcbrepov2 #9283
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9283 +/- ##
==========================================
- Coverage 70.48% 63.59% -6.90%
==========================================
Files 515 635 +120
Lines 23150 32773 +9623
==========================================
+ Hits 16317 20841 +4524
- Misses 5776 10328 +4552
- Partials 1057 1604 +547 ☔ View full report in Codecov by Sentry. |
|
|
||
| var RepositoryManagerClient = repositoryManagerClient | ||
|
|
||
| func GetRepoInfo(ctx context.Context, gcpProject, gcpRegion, gcpConnectionName, gcpRepoName string) (repoURI string, readAccessToken string, err error) { |
There was a problem hiding this comment.
I'm finding the current design of this function a bit difficult to follow. For a feature requiring multiple PRs, a top-down approach might make it easier to visualize how everything fits together. Though bottom-up approaches ensure solid unit tests, top-down can be more conducive to getting everyone on the same page – but I understand this preference can be subjective.
There was a problem hiding this comment.
Good point, it makes sense for the understanding of the full picture. I added a few more comments in the description of this PR aiming to clarify the next steps + full idea here. If you think is fine, we can keep this PR as it is, and we can use the top-down approach for future feature developments so we can express the full intention in a single PR, what do you think? Thanks 😄!
| gcpConnection string | ||
| gcpRepo string | ||
| expectedGitRepo string | ||
| expectedToken string |
There was a problem hiding this comment.
no non-zero values for test
There was a problem hiding this comment.
I changed the structure of the unit tests, also taking into account that now this package has the logic to build the oauth2 format. Let me know how it looks. Thanks!
|
|
||
| var RepositoryManagerClient = repositoryManagerClient | ||
|
|
||
| func GetRepoInfo(ctx context.Context, gcpProject, gcpRegion, gcpConnectionName, gcpRepoName string) (repoURI string, readAccessToken string, err error) { |
There was a problem hiding this comment.
can we use a struct for (repoURI string, readAccessToken string) , or we can also construct the uri with token here
https://oauth2:<token>@<repo>
There was a problem hiding this comment.
Got it! I changed it to return a struct as you suggested. The struct contains two fields, the CloneRepoURI with the oauth2 format, and the URI without the token. The reason to return both is to use the CloneRepoURI to execute all the needed git commands (clone, fetch, etc), and the URI for the errors we print so we don't show the token in the execution logs. The idea is to re use the logic that we already have in pkg/skaffold/git/gitutil.go to trigger the repo download/sync. Let me know what do you think.
renzodavid9
force-pushed
the
issue-9236-gcbrepov2-logic
branch
from
5206bcb
to
bab5cfb
Compare
Related: #9236
Description
cloud.google.com/go/cloudbuildlibrary to manage connection with GCB repos v2Follow-up Work (remove if N/A)
gcbrepov2.GetRepoInfowhen a GCB Repository V2 is configured in theskaffold.yamlfilegcbrepov2.GetRepoInfofunction; we'll need to update the parameters to receive a new type of object that supports both, the repo + clone repo URI