Fix - app deployment issue #154
Conversation
codegen.go
Outdated
| // Create a map to track used parameters | ||
| usedParams := make(map[string]bool) | ||
|
|
||
| addParameter := func(param string) string { |
There was a problem hiding this comment.
make this an independent function for cleanliness?
codegen.go
Outdated
| @@ -2167,6 +2208,10 @@ func FixFunctionName(functionName, actualPath string, lowercase bool) string { | |||
| functionName = strings.Replace(functionName, " ", "_", -1) | |||
| functionName = strings.Replace(functionName, "-", "_", -1) | |||
|
|
|||
| functionName = strings.Replace(functionName, "%", "", -1) | |||
There was a problem hiding this comment.
can you try handling this by whitelisting characters instead of blacklisting them? notice how:
- " " -> "" and "-" -> ""
- but, the rest are just stripped. we can't keep dealing with bullshit characters. might as well strip them away and start from a predictable charset (a-z, A-Z, 1-9)
@frikky any other reviews?
codegen.go
Outdated
| // Create a map to track used parameters | ||
| usedParams := make(map[string]bool) | ||
|
|
||
| addParameter := func(param string) string { |
There was a problem hiding this comment.
Why use an inline function? That is very hard to track compared to just jumping to the right function itself.
Also makes it not reusable.
codegen.go
Outdated
| @@ -747,14 +747,55 @@ func MakePythoncode(swagger *openapi3.Swagger, name, url, method string, paramet | |||
| // Extra param for url if it's changeable | |||
| // Extra param for authentication scheme(s) | |||
| // The last weird one is the body.. Tabs & spaces sucks. | |||
|
|
|||
| // Create a map to track used parameters | |||
| usedParams := make(map[string]bool) | |||
There was a problem hiding this comment.
I got a feeling this would crash somewhere due to how maps work.
codegen.go
Outdated
| paramName = strings.TrimSpace(paramName) | ||
|
|
||
| // If parameter hasn't been used before, add it | ||
| if !usedParams[paramName] { |
There was a problem hiding this comment.
Won't this ALWAYS crash the first time?
Don't you gotta do the if value, ok := usedParams[paramName]; ok {} ?
codegen.go
Outdated
| queryString, | ||
| bodyParameter, | ||
| verifyParam, | ||
| addParameter(strings.TrimPrefix(authenticationParameter, ",")), |
There was a problem hiding this comment.
I'm scared of this. Sounds like it would break literally everything.
|
In short: Your code is not at all trustworthy for our problem, unless it's tested across 1000+ apps and doesn't break a single one. I'm scared of deploying it due to it being VERY big changes to the actual function definitions. This does not feel like a good solution, as it's a solution on the code generation side (python) and not a solution on the control-mechanism of the incoming OpenAPI |
|
In these two commit ( commit 1, commit 2 ) made the following changes:
Most app deployments were failing due to these issues. To validate this changes, I deployed 8–10 different apps on Frankfurt, and it worked successfully. |
|
As this is related to App Health, I'm assigning it to you @yashsinghcodes. You are testing similar stuff already, and I need you to make sure we can push this code without it causing even a single old app to stop functioning. Changing names of actions in one place (python) but not another (app.yaml) have a tendency to create chaos. |
@frikky Please review once before merging