-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(spec): add support for health #143
Conversation
pkg/encoding/v1/encoding.go
Outdated
Liveness *api_v1.Probe | ||
} | ||
|
||
func interfaceToJSON(i interface{}) interface{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add a comment explaining what this actually does and why it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! actually moved this function in encoding/utils
pkg/encoding/v1/encoding.go
Outdated
case []interface{}: | ||
for i, v := range x { | ||
x[i] = interfaceToJSON(v) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if someone passes something else? This switch should have default
to catch that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The json package uses map[string]interface{} and []interface{} values to store arbitrary JSON objects and arrays; it will happily unmarshal any valid JSON blob into a plain interface{} value.
So we don't need anything else more. Source: https://blog.golang.org/json-and-go
pkg/encoding/v1/encoding.go
Outdated
return i | ||
} | ||
|
||
func interfaceToProbe(i interface{}) (*api_v1.Probe, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment, explaining what is a reason for this function.
Are there any limitations what on how interface should look like?
Is is possible to use something more specific instead of an empty interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment about this function as well, but I don't know how should I mention the exactness of the interface. I think it will fail to unmarshal into probe data structure and then we will know something is wrong with the way data is given.
pkg/encoding/v1/encoding.go
Outdated
@@ -219,11 +222,81 @@ func (m *Mount) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
return nil | |||
} | |||
|
|||
type Health struct { | |||
ReadinessProbe interface{} `yaml:"readinessProbe,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be an empty interface?
What is the difference between ReadinessProbe
and Readiness
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to have the latest changes as we discussed! Now it should be less confusing!
ecf9c2d
to
3d366a1
Compare
4045bde
to
322c3ff
Compare
@kadel @containscafeine PTAL at code need some help with adding docs, though, should I refer to the upstream docs directly or add all the fields in our doc? |
db57328
to
86a2731
Compare
🤣 |
i would add it into our doc, I think it would be better to have everything in one single doc. |
@@ -219,11 +222,83 @@ func (m *Mount) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
return nil | |||
} | |||
|
|||
type Health struct { | |||
// Data holder for ReadinessProbe while parsing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a bit more explaining.
What is a data holder? Why it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still missing WHY part of this.
We have to explain why we are doing it like this via interface{}
.
We will forget why it was done like this next time we are reading this.
I already forgot it and you explained it to me last week :-D
examples/wordpress/health.yaml
Outdated
health: | ||
readinessProbe: | ||
httpGet: | ||
port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this port: 80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
Add support for providing things like livenessProbe and readinessProbe. Add a new field in container level called health which has readinessProbe and livenessProbe. These fields are identical to the way we define livenessProbe or readinessProbe in kubernetes. So no new extra field has been added, everything that k8s supports works with the health option in OpenCompose. fixes redhat-developer#24
path: / | ||
port: 80 | ||
initialDelaySeconds: 5 | ||
timeoutSeconds: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@containscafeine updated as it is used in prod clusters!
Closing since the project is now discontinued! |
Add support for providing things like livenessProbe and readinessProbe. Add a new field in container level called health which has readinessProbe and livenessProbe. These fields are identical to the way we define
livenessProbe or readinessProbe in kubernetes. So no new extra field has been added, everything that k8s supports works with the health option in OpenCompose.
fixes #24