-
Notifications
You must be signed in to change notification settings - Fork 30
Onboard Intake(2/3): introduce Intake #1124
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
base: main
Are you sure you want to change the base?
Onboard Intake(2/3): introduce Intake #1124
Conversation
| func NewCmd(p *params.CmdParams) *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "create", | ||
| Short: "Creates a new Intake", |
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.
Just for my understanding: What is an "intake"? Is it kind of an instance (like it is for managed databases)?
Because I would maybe suggest something like stackit beta intake instance create then...
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.description, func(t *testing.T) { | ||
| p := print.NewPrinter() |
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.
Please use our new testutil funcs testutil.TestParseInput or testutil.TestParseInputWithAdditionalFlags to save us and yourself all the boilerplate code here.
See #1033 for reference.
stackit-cli/internal/pkg/testutils/testutils.go
Lines 14 to 22 in fb6cfca
| // TestParseInput centralizes the logic to test a combination of inputs (arguments, flags) for a cobra command | |
| func TestParseInput[T any](t *testing.T, cmdFactory func(*types.CmdParams) *cobra.Command, parseInputFunc func(*print.Printer, *cobra.Command, []string) (T, error), expectedModel T, argValues []string, flagValues map[string]string, isValid bool) { | |
| TestParseInputWithAdditionalFlags(t, cmdFactory, parseInputFunc, expectedModel, argValues, flagValues, map[string][]string{}, isValid) | |
| } | |
| // TestParseInputWithAdditionalFlags centralizes the logic to test a combination of inputs (arguments, flags) for a cobra command. | |
| // It allows to pass multiple instances of a single flag to the cobra command using the `additionalFlagValues` parameter. | |
| func TestParseInputWithAdditionalFlags[T any](t *testing.T, cmdFactory func(*types.CmdParams) *cobra.Command, parseInputFunc func(*print.Printer, *cobra.Command, []string) (T, error), expectedModel T, argValues []string, flagValues map[string]string, additionalFlagValues map[string][]string, isValid bool) { | |
| p := print.NewPrinter() |
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.
Or is there any reason why you're not using it? Then please tell me, so we can fix potential issues 😅
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.description, func(t *testing.T) { | ||
| p := print.NewPrinter() |
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.
same here, please use the testutil func
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.description, func(t *testing.T) { | ||
| p := print.NewPrinter() |
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.
same here, please use the testutil func
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.description, func(t *testing.T) { | ||
| p := print.NewPrinter() |
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.
same here, please use the testutil func
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.description, func(t *testing.T) { | ||
| testutils.TestParseInput(t, NewCmd, parseInput, tt.expectedModel, tt.argValues, tt.flagValues, tt.isValid) |
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.
Why are you using the testutils.TestParseInput func here but not for your other implementations? 😅
This is the way, please use it everywhere.
| } | ||
|
|
||
| if !model.AssumeYes { | ||
| prompt := fmt.Sprintf("Are you sure you want to create an Intake for project %q?", projectLabel) |
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.
| prompt := fmt.Sprintf("Are you sure you want to create an Intake for project %q?", projectLabel) | |
| prompt := fmt.Sprintf("Are you sure you want to create an Intake instance for project %q?", projectLabel) |
In case it's an instance, I would phrase it like this
| if model.Async { | ||
| operationState = "Triggered deletion of" | ||
| } | ||
| p.Printer.Info("%s stackit Intake instance %s \n", operationState, model.IntakeId) |
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.
| p.Printer.Info("%s stackit Intake instance %s \n", operationState, model.IntakeId) | |
| p.Printer.Outputf("%s stackit Intake instance %s \n", operationState, model.IntakeId) |
p.Printer.Info would log to stderr, p.Printer.Outputf logs to stdout. I guess you want log to stdout here 😅
|
@devanshcache please note: All of this also applies to #1126 |
Description
Intake is composed of three components:
Intake runner has already been merged reference (#952) this PR is continuation of the one of two components of Intake (ticket)
Checklist
make fmtmake generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)