-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update RevisionSpec to use corev1.Container instead of ContainerSpec #174
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
Conversation
mattmoor
left a comment
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 is awesome. We should probably have follow-up issues for validating:
- If there are features of
corev1.Containerwe want to disallow, we should validate them out. - Add testing that we properly pass through features like
ReadinessCheck.
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 believe we can remove these too. Is anything keying off of the options these contain right now?
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'll do this in a follow-up CL.
pkg/controller/revision/ela_pod.go
Outdated
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 think that this should almost entirely be pass-thru.
Can we start with: elaContainer := u.Spec.ContainerSpec.DeepCopy() and mutate it with our additions?
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.
pkg/webhook/webhook_test.go
Outdated
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 can be abbreviated as:
Env: []corev1.EnvVar{}
Name: envVarName,
Value: envVarValue,
}},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.
This was mostly a drop-in replacement, except that I updated the Env specification to use corev1.Container.Env instead of being a top-level field on RevisionSpec. Fixes #17.
Resolve only v1alpha1 CRDs until we're on OpenShift 4.2.
This was mostly a drop-in replacement, except that I updated the Env
specification to use corev1.Container.Env instead of being a top-level
field on RevisionSpec.
Fixes #17.