Skip to content
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

Update RevisionSpec to use corev1.Container instead of ContainerSpec #174

Merged
merged 3 commits into from
Feb 14, 2018

Conversation

drewinglis
Copy link
Contributor

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.

Copy link
Member

@mattmoor mattmoor left a 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:

  1. If there are features of corev1.Container we want to disallow, we should validate them out.
  2. Add testing that we properly pass through features like ReadinessCheck.

@@ -76,13 +72,7 @@ type RevisionSpec struct {
FunctionSpec *FunctionSpec `json:"functionSpec,omitempty"`
AppSpec *AppSpec `json:"appSpec,omitempty"`
Copy link
Member

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?

Copy link
Contributor Author

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.

@@ -55,7 +55,7 @@ func MakeElaPodSpec(u *v1alpha1.Revision) *corev1.PodSpec {
Name: "health-checks",
},
},
Env: u.Spec.Env,
Env: u.Spec.ContainerSpec.Env,
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Name: envVarName,
Value: envVarValue,
Env: []corev1.EnvVar{
corev1.EnvVar{
Copy link
Member

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,
}},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@drewinglis
Copy link
Contributor Author

Filed #175 and #176 as follow-up issues.

Drew Inglis added 3 commits February 14, 2018 13:25
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.
@drewinglis drewinglis merged commit 7a787ef into knative:master Feb 14, 2018
@drewinglis drewinglis deleted the corev1 branch February 14, 2018 21:28
lberk pushed a commit to lberk/serving that referenced this pull request Jul 4, 2019
Resolve only v1alpha1 CRDs until we're on OpenShift 4.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants