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.

Sign up for GitHub

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

Jump to bottom

Validate service accounts in revision #4733

Merged
merged 1 commit into from
Jul 13, 2019
Merged

Validate service accounts in revision #4733

merged 1 commit into from
Jul 13, 2019

Conversation

shashwathi
Copy link
Contributor

Fixes #4647

Proposed Changes

  • Add validation for service accounts in revision spec

Release Note

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 12, 2019
@knative-prow-robot knative-prow-robot added area/API API objects and controllers size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 12, 2019
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/serving/k8s_validation.go 98.9% 99.0% 0.0

taragu
taragu reviewed Jul 12, 2019
View reviewed changes
pkg/apis/serving/k8s_validation_test.go
@@ -170,6 +170,15 @@ func TestPodSpecValidation(t *testing.T) {
}},
},
want: apis.ErrDisallowedFields("initContainers"),
}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this test is passing (because missing container check comes before service account check), could we update the service account name here for completeness?

		name: "missing container",
		ps: corev1.PodSpec{
			ServiceAccountName: "bob",  // to a valid service account name
			Containers:         []corev1.Container{},
		},
		want: apis.ErrMissingField("containers"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bob is a valid service account name. I do not think there is anything to update in this test. Am I missing something?

@mattmoor mattmoor self-assigned this Jul 12, 2019
mattmoor
mattmoor reviewed Jul 13, 2019
View reviewed changes
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.

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2019
@knative-prow-robot knative-prow-robot merged commit 8e50a8e into knative:master Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taragu taragu taragu left review comments

@mattmoor mattmoor mattmoor left review comments

@vaikas vaikas Awaiting requested review from vaikas

Assignees

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate that service accounts have the right shape
6 participants
@shashwathi @knative-metrics-robot @knative-prow-robot @mattmoor @taragu @googlebot