-
Notifications
You must be signed in to change notification settings - Fork 79
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
Finish grpc create app #207
Conversation
cmd/client/cmd/app.go
Outdated
if team == "" { | ||
fmt.Fprintln(os.Stderr, "You must provide the name of the team") | ||
return | ||
} |
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.
what about simplify this validation, like this:
team, err := cmd.Flags().GetString("team")
if err != nil || team == "" {
fmt.Fprintln(os.Stderr, "You must provide a valid name of the team")
return
}
?
(or just ignore error and only check for blank team name)
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.
Your snippet is better.
cmd/client/cmd/app.go
Outdated
} | ||
|
||
scaleMin, _ := cmd.Flags().GetInt32("scale-min") | ||
if err != nil { |
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.
- scaleMin, _ := cmd.Flags().GetInt32("scale-min")
+ scaleMin, err := cmd.Flags().GetInt32("scale-min")
Max: int64(666), | ||
Min: int64(1), | ||
CpuTargetUtilization: 42, | ||
Max: 666, |
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.
👹
ObjectMeta: k8sv1.ObjectMeta{ | ||
Name: a.Name, | ||
Labels: map[string]string{ | ||
"teresa.io/team": a.Team, |
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.
Maybe I found a bug on #205 .
I was getting app team for teresa.io/app
annotation 😕 .
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.
We talked and settled for using the label.
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.
BTW, fixed
8f7402d
to
7c38bb6
Compare
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.
👏
7c38bb6
to
6ccee3f
Compare
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
pkg/server/app/app.go
Outdated
CreateNamespace(*App, string) error | ||
CreateQuota(*App) error | ||
CreateSecret(string, string, map[string][]byte) error | ||
CreateAutoScale(*App) error |
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.
💅 maybe a simple name for these args helps to understand they proposal 💅
pkg/server/app/app_test.go
Outdated
|
||
if err := ops.Create(user, app); err == nil { | ||
t.Errorf("expected error, got nil") | ||
} |
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.
- if err := ops.Create(user, app); err == nil {
+ if ops.Create(user, app) == nil {
pkg/server/app/app_test.go
Outdated
} | ||
ops.(*AppOperations).kops = &errK8sOperations{QuotaErr: errors.New("Quota Error")} | ||
|
||
if err := ops.Create(user, app); err == nil { |
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.
- if err := ops.Create(user, app); err == nil {
+ if ops.Create(user, app) == nil {
pkg/server/app/app_test.go
Outdated
} | ||
ops.(*AppOperations).kops = &errK8sOperations{AutoScaleErr: errors.New("AutoScale Error")} | ||
|
||
if err := ops.Create(user, app); err == nil { |
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.
- if err := ops.Create(user, app); err == nil {
+ if ops.Create(user, app) == nil {
6ccee3f
to
a84c712
Compare
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.
🚀
For now I'm postponing error translation and setting the defaults on the server as this PR is already too big.
Related #203.
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)