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

feat+fix: modify configs and refactor cli #18

Merged
merged 2 commits into from
Jul 9, 2023
Merged

feat+fix: modify configs and refactor cli #18

merged 2 commits into from
Jul 9, 2023

Conversation

zenhorace
Copy link
Contributor

This PR does a lot, in short:

  • Refactor the CLI to make it more idiomatic Go and make it easier to extend in future development
  • The overhaul also fixed Generated configs fail to marshal #14 and enabled
  • Update command, which takes an input file, allows adding/updating services and outputs a new file with the merger of changes.

I know it's a lot (and you may not like the approach), so take your time, and we can discuss it.

Copy link
Contributor

@KyleTryon KyleTryon left a comment

Choose a reason for hiding this comment

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

Massively appreciate everything you did here @zenhorace. 🙏 This helped me understand a few things about Go and really set us up for a much better foundation moving forward. Huge thanks to you.

@@ -10,14 +10,14 @@ import (

"github.com/AlecAivazis/survey/v2"
"github.com/spf13/cobra"
"github.com/techsquidtv/uhs-cli/cmd/common"
configCommon "github.com/techsquidtv/uhs-cli/models/common"
"github.com/techsquidtv/uhs-cli/cmd/shared"
Copy link
Contributor

Choose a reason for hiding this comment

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

love the shared change!

if !ok {
return nil, fmt.Errorf("unregistered service name: %q", key)
}
sc := v()
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice 👍

@KyleTryon
Copy link
Contributor

I'm going to accept everything as it is now, it won't get tagged right away, so we have time to change things before then.
Let me see if i can grab these errors real quick (if you allowed pushing to fork)

@KyleTryon KyleTryon merged commit 10118d8 into TechSquidTV:main Jul 9, 2023
0 of 2 checks passed
@KyleTryon
Copy link
Contributor

welp i messed something up there, that was a mistake. instead of pushing to your main i did mine somehow. all cool.. going to fix this retroactively real fast. not the best outcome.

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.

Generated configs fail to marshal
2 participants