App Deploy (gRPC series) #211
Conversation
cmd/client/cmd/deploy.go
Outdated
|
|
||
| currentClusterName, err := getCurrentClusterName() | ||
| if err != nil { | ||
| fmt.Fprintln(os.Stderr, "error on read config file:", err) |
There was a problem hiding this comment.
I guess "error reading config file is better"
cmd/client/cmd/deploy.go
Outdated
| } | ||
| if n == 0 { | ||
| break | ||
| } |
There was a problem hiding this comment.
Yeah, much better.
pkg/server/app/app.go
Outdated
| @@ -19,6 +19,9 @@ type Operations interface { | |||
| Create(user *storage.User, app *App) error | |||
| Logs(user *storage.User, appName string, lines int64, follow bool) (io.ReadCloser, error) | |||
| Info(user *storage.User, appName string) (*Info, error) | |||
| TeamName(appName string) (string, error) | |||
| Meta(appName string) (*App, error) | |||
| HasPermission(user *storage.User, appName string) bool | |||
There was a problem hiding this comment.
I guess the name Meta is a little bit misleading as it returns an app instance but I can't think of something better now.
There was a problem hiding this comment.
Me too.
I'll think again about this name, but for now, I don't have any idea.
There was a problem hiding this comment.
Let me know if you find a better name.
There was a problem hiding this comment.
What do you think about Get ?
I'm not sure because that method returns only information about the app, not a powerful struct with useful methods. BUT, is better than Meta? (or not? 🤔 )
There was a problem hiding this comment.
It' better, perhaps Instance.
|
|
||
| func TestNewBuildSpec(t *testing.T) { | ||
| expectedDeployId := "123" | ||
| expectedTarBallLocation := "narnia" |
pkg/server/deploy/deploy.go
Outdated
| go func() { | ||
| defer w.Close() | ||
| if err = ops.buildApp(tarBall, a, deployId, buildDest, w); err != nil { | ||
| return |
There was a problem hiding this comment.
Maybe we should log the buildApp error
There was a problem hiding this comment.
Oh, I forgot :/
| scanner := bufio.NewScanner(r) | ||
| for scanner.Scan() { | ||
| c <- fmt.Sprintln(scanner.Text()) | ||
| } |
There was a problem hiding this comment.
I guess we should check scanner.Err() at the end of the loop, not sure though.
There was a problem hiding this comment.
If raised an error we can't do much more than log the error :/
pkg/server/deploy/handlers.go
Outdated
| } | ||
| defer rc.Close() | ||
|
|
||
| deployMsgs := sendMsgsToAChannel(rc) |
There was a problem hiding this comment.
Maybe rename sendMsgsToAChannel -> channelFromReader
There was a problem hiding this comment.
😱 🏆 This name is much better on a monumental scale.
pkg/server/k8s/client.go
Outdated
| } | ||
| io.Copy(w, stream) | ||
|
|
||
| if err = k.waitPodEnd(pod, 1*time.Second, 5*time.Minute); err != nil { |
There was a problem hiding this comment.
For deploy safety we need more than 5 minutes of finish timeout, maybe more args. Also we may increase the poll interval (less cpu intensive)
There was a problem hiding this comment.
Hm, What do you think about 30 minutes for finish timeout and 3 seconds for check interval?
pkg/server/k8s/client.go
Outdated
| return int(state.ExitCode) | ||
| } | ||
| } | ||
| return 0 |
There was a problem hiding this comment.
Maybe return error if the pod is running
There was a problem hiding this comment.
Do you say that because Terminated is a pointer and it can be nil?
I'll prevent this.
There was a problem hiding this comment.
If I call this function on a running pod I would expect an error: pod is still running, no exit code yet. But this is 💅
There was a problem hiding this comment.
Gotcha!!! I'll change
pkg/server/k8s/client.go
Outdated
| func newInClusterK8sClient() (Client, error) { | ||
| conf, err := restclient.InClusterConfig() | ||
| func (k *k8sClient) CreateDeploy(deploySpec *deploy.DeploySpec) error { | ||
| replicas := k.currentPodReplicasFromDeploy(deploySpec.Namespace, deploySpec.Name) |
There was a problem hiding this comment.
Maybe a django style name CreateOrUpdateDeploy
There was a problem hiding this comment.
I don't like functions with more than one proposal but you're right, the current name does not reflect the reality.
…ement K8S methods to deal with deploy
… limit per deploy
drgarcia1986
force-pushed
the
dg-app_deploy-grpc
branch
from
80a36b5
to
07c25f8
Compare
related #203
It's a big pull request with tons of code change on sensible features, please, pay attention and don't hesitate to point errors or even improvements.
BTW, now Teresa will delete "build pods" after they finish their job.
Good Luck 🇯🇲
This change is