merge DeploymentSpec with PodSpec on the top level #36
Conversation
| }, | ||
| func createDeployment(app *spec.App) (*ext_v1beta1.Deployment, error) { | ||
|
|
||
| // We are merging whole DeploymentSpect with PodSpec. |
| } | ||
|
|
||
| // TODO: check if this wasn't set by user, in that case we shouldn't ovewrite it |
There was a problem hiding this comment.
Do we want to do this in this PR or later?
There was a problem hiding this comment.
I wanted to handle this more systematically.
It is a bit ralted to your PR #37
|
|
||
| // use top level name and labels for deployment | ||
| deployment.ObjectMeta.Name = app.Name | ||
| deployment.ObjectMeta.Labels = app.Labels |
There was a problem hiding this comment.
Can we do this like we were doing before -
&ext_v1beta1.Deployment{
- ObjectMeta: api_v1.ObjectMeta{
- Name: app.Name,
- Labels: app.Labels,
- },
- Spec: ext_v1beta1.DeploymentSpec{
- Replicas: app.Replicas,
it's a bit more readable and consistent with the rest of the code, WDYT?
There was a problem hiding this comment.
I think this is far more readable. At least for me. Otherwise, it can get quite messy with deep indentation.
But more importantly, if you have it like this it allows you to add additional logic more easily.
There was a problem hiding this comment.
When we do the deep indentation, the code actually looks a bit the objects that will get generated, which is helpful while reading.
Also, everywhere we have done it with the indentation way.
And, the object to which we want to add additional logic, we create it separately and substitute it in the indentation.
Idk, I really like the indentation way :( a lot more readable.
@surajssd WDYT?
There was a problem hiding this comment.
if you both agree, I will change it
There was a problem hiding this comment.
what do you think @surajssd ?
There was a problem hiding this comment.
I'll have to agree with @containscafeine that the previous way looks more straight-forward.
|
This LGTM, merge when you feel @containscafeine 's concerns are addressed! |
|
|
||
| // use top level name and labels for deployment | ||
| deployment.ObjectMeta.Name = app.Name | ||
| deployment.ObjectMeta.Labels = app.Labels |
There was a problem hiding this comment.
I'll have to agree with @containscafeine that the previous way looks more straight-forward.
| deployment, err := createDeployment(app) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "app %q", app.Name) | ||
| } |
|
updated. I can't do everything like that as deplymentSpec needs to be modified and I can't do that if I create new one. |
fixes #35
There is more checks that needs to be done, I've added TODO for few that I could think of while writing this.