Skip to content
This repository was archived by the owner on Aug 29, 2023. It is now read-only.

Conversation

luxas
Copy link
Contributor

@luxas luxas commented Jun 10, 2020

Using the knownhosts package here too so we can easily validate remotes.
Thank you for making this work totally in-memory 😄!

Upgraded to Go 1.14, as fluxcd/toolkit depends on k8s v1.18 which requires newer than 1.12

/cc @stefanprodan @hiddeco
/assign @twelho

Copy link
Contributor

@twelho twelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One little nit in the logic, otherwise LGTM 👍 very good!

@@ -94,13 +99,19 @@ func NewGitDirectory(url string, opts GitDirectoryOptions) (*GitDirectory, error
switch ep.Protocol {
case "ssh":
// If we haven't got the right credentials, just continue in read-only mode
if len(opts.IdentityFileContent) == 0 {
if len(opts.IdentityFileContent) == 0 && len(opts.KnownHostsFileContent) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably should be an OR-case? You need both the identity file and known hosts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch, will fix!

@stefanprodan
Copy link

I think we should drop the vendor dir, go modules have matured and you can easily cache them in CI e.g. https://github.com/fluxcd/toolkit/blob/master/.github/workflows/e2e.yaml#L16-L21

@luxas
Copy link
Contributor Author

luxas commented Jun 11, 2020

I think we should drop the vendor dir, go modules have matured and you can easily cache them in CI e.g. https://github.com/fluxcd/toolkit/blob/master/.github/workflows/e2e.yaml#L16-L21

Thanks, will/can do, in a future PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants