Skip to content

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.

@drewinglis drewinglis requested a review from mattmoor February 14, 2018 20:57
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.

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.

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.

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