-
Notifications
You must be signed in to change notification settings - Fork 448
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
Cobra cli #69
Cobra cli #69
Conversation
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Fix #54 |
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.
Thanks for your contribution.
It is too late in Shanghai, China (23:30) and I just look through the code. Generally LGTM but I prefer to review it tomorrow if it does not block you.
cmd/cli/create.go
Outdated
"github.com/spf13/cobra" | ||
) | ||
|
||
//NewCommandGet generate run cmd |
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.
The function name is NewCommandCreate
. Maybe there is a typo
cmd/cli/push.go
Outdated
"github.com/spf13/cobra" | ||
) | ||
|
||
//NewCommandGet generate run cmd |
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 problem above.
@gaocegege Thank you! |
cmd/cli/get-model.go
Outdated
var sInfo []*api.StudyInfo | ||
for _, si := range r.StudyInfos { | ||
if len(opt.args) > 0 { | ||
if utf8.RuneCountInString(opt.args[0]) >= 7 { |
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.
Could we add some comments here? I can not understand the code well.
cmd/cli/get.go
Outdated
cmd.AddCommand(NewCommandGetStudies()) | ||
cmd.AddCommand(NewCommandGetStudy()) | ||
// cmd.AddCommand(NewCommandGetTrials()) | ||
// cmd.AddCommand(NewCommandGetTrial()) |
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.
I am wondering if we could enable the two command. WDYT
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.
Unfortunately, we don't have the get trial API in katib-manager.
I will refactor API and add low-level APIs (related #66 ). Then I will enable these commands.
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.
SGTM, I misunderstand the comments.
fmt.Println(err) | ||
os.Exit(1) | ||
} | ||
|
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 remove the line here.
cmd/cli/push-model.go
Outdated
args []string | ||
} | ||
|
||
//NewCommandGetStudy generate run cmd |
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 update or remove the comment here.
Signed-off-by: YujiOshima <[email protected]>
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.
/lgtm
cmd/cli/get.go
Outdated
cmd.AddCommand(NewCommandGetStudies()) | ||
cmd.AddCommand(NewCommandGetStudy()) | ||
// cmd.AddCommand(NewCommandGetTrials()) | ||
// cmd.AddCommand(NewCommandGetTrial()) |
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.
SGTM, I misunderstand the comments.
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
@gaocegege At last, it passed the CI! |
/lgtm 🎉 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaocegege The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Introduce
spf13/cobra
to katib-cli.CLI schema is drastically changed.
/assign @gaocegege