Skip to content
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

go-git implementation #233

Merged
merged 31 commits into from
Jun 4, 2021
Merged

go-git implementation #233

merged 31 commits into from
Jun 4, 2021

Conversation

luizbafilho
Copy link
Contributor

@luizbafilho luizbafilho commented Jun 1, 2021

What changed?
Moving all git cli calls to go-git library.

Why?
It'll make it easier to mock and maintain the code

How did you test it?

Release notes

Documentation Changes

@luizbafilho luizbafilho marked this pull request as ready for review June 3, 2021 14:07
@luizbafilho luizbafilho changed the title Initial go-git impl go-git implementation Jun 3, 2021
cmd/wego/add/cmd.go Show resolved Hide resolved
pkg/cmdimpl/add.go Outdated Show resolved Hide resolved
pkg/cmdimpl/add.go Show resolved Hide resolved
pkg/cmdimpl/add.go Outdated Show resolved Hide resolved
@@ -15,22 +13,6 @@ import (

// Status provides the implementation for the wego status application command
func Status(allParams AddParamSet) error {
// verify the app is in the wego apps folder
Copy link
Contributor

Choose a reason for hiding this comment

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

This removes the ability to let the user know the possibility of an unexcited app.

pkg/git/gogit.go Show resolved Hide resolved
pkg/git/gogit.go Show resolved Hide resolved
@josecordaz
Copy link
Contributor

maybe a bit more coverage ? 😬

Copy link
Contributor

@jrryjcksn jrryjcksn left a comment

Choose a reason for hiding this comment

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

VERY much cleaner overall!

cmd/wego/add/cmd.go Show resolved Hide resolved
cmd/wego/add/cmd.go Show resolved Hide resolved
@@ -42,7 +44,19 @@ func init() {

func runCmd(cmd *cobra.Command, args []string) {
params.Namespace, _ = cmd.Parent().Flags().GetString("namespace")
if err := cmdimpl.Add(args, params); err != nil {

authMethod, err := ssh.NewPublicKeysFromFile("git", params.PrivateKey, params.PrivateKeyPass)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this taking a password on the command line? That's usually frowned on since it's available in /proc. Normally, it's considered good practice to either pull from an environment variable or prompt for a password.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Just took the same approach flux does, but we can change it. Do you mind if I do it in another PR? I'd like to get this one merged asap.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. No problem

@@ -314,79 +333,63 @@ func Add(args []string, allParams AddParamSet) error {
fmt.Printf("%s\n\n", clusterStatus)

if clusterStatus == status.Unmodified {
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're in here, could you make this a switch and add a check for status.Unknown? Iftikhar ran into this yesterday. We should exit at this point if we can't talk to the cluster.

Copy link
Contributor

@jrryjcksn jrryjcksn left a comment

Choose a reason for hiding this comment

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

LGTM

@luizbafilho luizbafilho merged commit c95c20f into main Jun 4, 2021
@luizbafilho luizbafilho deleted the 140/go-git branch June 4, 2021 12:18
@luizbafilho luizbafilho restored the 140/go-git branch December 8, 2021 17:35
@bigkevmcd bigkevmcd deleted the 140/go-git branch December 15, 2021 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants