Expose method and expected status code on http checks #122
Conversation
There was a problem hiding this comment.
Hey @gkze,
Thanks for your PR!!
Look really good!
There are a few minor requests:
- The transition from
http_gettohttpbreaks backward compatibility. Can we support both? We can say that it will be deprecated later with a code that I shared with you in the comment. - Can you please add tests that validate the new default values
GETand200. - Can you please add the Method to the renderer?
- I think this new and shiny functionality deserves documentation.
Cheers!
| "net/url" | ||
| "strings" | ||
| ) | ||
|
|
||
| type Probe struct { | ||
| Exec *ExecProbe `yaml:"exec,omitempty"` | ||
| HttpGet *HttpProbe `yaml:"http_get,omitempty"` | ||
| Http *HttpProbe `yaml:"http,omitempty"` |
There was a problem hiding this comment.
This breaks backward compatibility.
Do you think we can have both http and http_get fields at the same time?
Even nicer would be to use the deprecation warning:
https://github.com/F1bonacc1/process-compose/blob/main/src/types/deprecation.go
There was a problem hiding this comment.
Thanks, will do. Can you give an example of how this is done?
There was a problem hiding this comment.
Certainly.
Check out this commit as an example:
45fceb3
I mistakenly used on-failure instead of on_failure, once discovered I kept both of them:
- I issued a warning during the first month after deprecation.
- Error + 5s sleep during the second month
- Exit 1 after that.
I would go ahead and add the deprecationCheck to validators.go and assign the http_get values (if exist) to http (if not defined) in mutators.go.
If that's too many steps, let me know and I will add this content.
There was a problem hiding this comment.
Sorry for the delay just getting back to this. I now realize that leaving both http_get and http we would need to make them mutually exclusive right? What should the behavior be in that case? I'm thinking failing parsing / validation if both of them are set, so that would mean changing the signature of Probe.validateAndSetDefaults(). Let me know your thoughts.
There was a problem hiding this comment.
IMO in case both of them are defined you can fail the loading in loader.validators.go.
Please let me know if you need any help with this feature.
|
No description provided.