Repository navigation
Conversation
| payloadValue := flags.FlagToStringPointer(p, cmd, payloadFlag) | ||
| var payload *sca.CreateApplicationPayload | ||
| if payloadValue != nil { | ||
| payload = &sca.CreateApplicationPayload{} | ||
| err := json.Unmarshal([]byte(*payloadValue), payload) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("enconde payload: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
docs state default payload is used when none is set - this is not the case
SerseusWasTaken
left a comment
SerseusWasTaken
left a comment
There was a problem hiding this comment.
last few nitpicks from my side, sorry! i'll hand this over to second review now
| // Call API | ||
| req, err := buildRequest(ctx, model, apiClient, current) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| resp, err := req.Execute() | ||
| if err != nil { | ||
| return fmt.Errorf("update application: %w", err) | ||
| } |
There was a problem hiding this comment.
update misses prompt before actually updating
| // Call API | ||
| req := buildRequest(ctx, model, apiClient) | ||
| resp, err := req.Execute() | ||
| if err != nil { | ||
| return fmt.Errorf("create application: %w", err) | ||
| } |
There was a problem hiding this comment.
please also add a prompt here before creating the resource
|
@JYisus, thanks for your contribution. One general comment: |
| network := &sca.Network{} | ||
|
|
||
| if model.Public != nil { | ||
| network.PublicIngress = *model.Public |
There was a problem hiding this comment.
field PublicIngress booljson:"publicIngress" of Network type
this defaults to false. So when creating an application with publicIngress: true, port: 8080 and the running an update, setting just the port publicIngress: false will be sent
| found := false | ||
| updatedRules := make([]sca.ScaleRule, 0, len(current)) | ||
|
|
||
| var httpRule *sca.HttpScaleRule |
There was a problem hiding this comment.
I'd move this into the loop, subsequent iterations can observe the value of httpRule from previous iterations.
| if model.Commands != nil { | ||
| containers[0].Command = model.Commands | ||
| } | ||
|
|
||
| if model.Args != nil { | ||
| containers[0].Args = model.Args | ||
| } |
There was a problem hiding this comment.
these assignments have no effect, maybe updateContainer = true is missing? They mutate the containers argument, but call sites of this function only use the return value
| if model.ScalingType == nil && | ||
| model.Instances == nil && | ||
| model.MinInstances == nil && | ||
| model.MaxInstances == nil && | ||
| model.RPS == nil && | ||
| model.Concurrency == nil { |
There was a problem hiding this comment.
model.ScaleToZero is used in buildAutoscalingConfig, which is called from this function. I'd suspect that application update --scale-to-zero=true would have no effect.
| if len(c.Command) > 0 { | ||
| cArgs = strings.Join(c.Args, " ") |
There was a problem hiding this comment.
| if len(c.Command) > 0 { | |
| cArgs = strings.Join(c.Args, " ") | |
| if len(c.Args) > 0 { | |
| cArgs = strings.Join(c.Args, " ") |
looks like copy paste error
4 participants
Description
relates to #1234
Checklist
make fmtmake generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)