-
Notifications
You must be signed in to change notification settings - Fork 168
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
Use go git #161
Use go git #161
Conversation
6195ccd
to
ae7c0fb
Compare
opts := &git.FetchOptions{ | ||
RefSpecs: []config.RefSpec{"refs/*:refs/*"}, | ||
Auth: resolvedAuth, | ||
Depth: 0, |
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.
Of note, For simplicity in this PR: I removed the depth=1 checkout from our current source-init. It would make sense in a future issue to optimize this checkout.
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.
Lots of really positive changes in this PR!
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.
Looks Good, Just added some questions and comments
@@ -1,4 +1,4 @@ | |||
package secret_test | |||
package dockercreds |
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.
Are we testing anything internal in this package? if not can we change the package to dockercreds_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.
We are not currently exporting the k8sGitKeychain. I would like to keep this in the dockercreds package.
f274fef
to
c250e7a
Compare
@@ -11,6 +11,8 @@ import ( | |||
) | |||
|
|||
const ( | |||
buildInitBinary = "/layers/org.cloudfoundry.go-mod/app-binary/build-init" // Can be changed to build-init in https://github.com/cloudfoundry/go-mod-cnb/issues/8 |
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.
Cant wait for the go buildpack to start putting the binaries on the path!!!!
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.
This PR sparks joy. We should test out a few of the scenarios the PAs are running into, but this is SOO GOOD
- combine source-init,creds-init,build-init - Mount volumes as CNB user to avoid needing to chown - Run Build Init as CNB user - Move k8sSecretKeychainFactory -> dockercreds package
c250e7a
to
84beea9
Compare
Use go-git to fetch git source builds