-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: new repo resolver logic to fetch info from a gcbrepov2 #9283
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no non-zero values for test
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5206bcb
to
bab5cfb
Compare
Related: #9236
Description
cloud.google.com/go/cloudbuild
library to manage connection with GCB repos v2Follow-up Work (remove if N/A)
gcbrepov2.GetRepoInfo
when a GCB Repository V2 is configured in theskaffold.yaml
filegcbrepov2.GetRepoInfo
function; we'll need to update the parameters to receive a new type of object that supports both, the repo + clone repo URI