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

Adding details to Conformance Tests using RFC 2119 standards. #60549

Merged
merged 1 commit into from Jul 8, 2018

Conversation

Projects
None yet
5 participants
@brahmaroutu
Copy link
Contributor

brahmaroutu commented Feb 28, 2018

This PR is part of the conformance documentation. This is to provide more formal specification using RFC 2119 keywords to describe the test so that who ever is running conformance tests do not have to go through the code to understand why and what is tested.
The documentation information added here into each of the tests eventually result into a document which is currently checked in at location https://github.com/cncf/k8s-conformance/blob/master/docs/KubeConformance-1.9.md

I would like to have this PR reviewed for v1.10 as I consider it important to strengthen the conformance documents.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 28, 2018

@brahmaroutu: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Mar 1, 2018

/ok-to-test

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Mar 1, 2018

flake is #60614

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from 6156cd8 to 59419c9 Mar 1, 2018

@brahmaroutu

This comment has been minimized.

Copy link
Contributor Author

brahmaroutu commented Mar 1, 2018

@WilliamDenniss @duglin @bradtopol Please review conformance documentation.

@@ -153,6 +153,11 @@ var _ = SIGDescribe("ServiceAccounts", func() {
}
})

/*
Release : v1.9

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

nit: extra space before :

This comment has been minimized.

@brahmaroutu

brahmaroutu Jul 5, 2018

Author Contributor

fixed

/*
Release : v1.9
Testname: Service Account Tokens Must AutoMount
Description: Ensure that Service Account keys are mounted into the Pod. Pod contains three containers each will read Service Account Key, root CA Key and Namespace Key respectively from the default API Token Mount path. All these three files MUST exist and the Service Account mount path MUST be auto mounted to the Pod.

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

Please wrap this line around 80-100 characters

This comment has been minimized.

@brahmaroutu

brahmaroutu Jul 5, 2018

Author Contributor

I think gofmt was messing up with some of the indentation, it seem to be smart with comment body too. Please see if I addressed all your concerns.

/*
Release : v1.9
Testname: Service account tokens auto mount optionally
Description: Ensure that Service Account keys are mounted into the Pod only when AutoMountServiceToken is not set to false. We test the following scenarios here.

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

ditto: please wrap

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch 2 times, most recently from 3f04566 to 6082ead Apr 28, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Apr 28, 2018

@brahmaroutu

This comment has been minimized.

Copy link
Contributor Author

brahmaroutu commented Apr 28, 2018

@tallclair Please review.

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from 6082ead to f1f3700 Apr 28, 2018

three containers each will read Service Account Key, root CA Key and Namespace Key respectively from
the default API Token Mount path. All these three files MUST exist and the Service Account mount
path MUST be auto mounted to the Pod.
*/

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

Please fix the indentation.

This comment has been minimized.

@tallclair

tallclair Jun 8, 2018

Member

Still not fixed

This comment has been minimized.

@brahmaroutu

brahmaroutu Jun 18, 2018

Author Contributor

Sorry about that.

This comment has been minimized.

@tallclair

tallclair Jun 27, 2018

Member

Still not fixed.

This comment has been minimized.

@brahmaroutu

brahmaroutu Jul 5, 2018

Author Contributor

fixed.

@@ -153,6 +153,14 @@ var _ = SIGDescribe("ServiceAccounts", func() {
}
})

/*
Release: v1.9
Testname: Service Account Tokens Must AutoMount

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

Why is the documented name different from the code description, i.e. "ServiceAccounts should mount an API token into pods". Seems like they should be consistent...

This comment has been minimized.

@brahmaroutu

brahmaroutu May 15, 2018

Author Contributor

@tallclair I was just trying to generalize all 'service account token' related tests with that prefix. Please suggest better wording.
Service Account Tokens - must auto mount
Service Account Tokens - auto mount optionally ...

This comment has been minimized.

@tallclair

tallclair Jun 8, 2018

Member

Lots conformance tests replace spaces with dashes in the testname. Is the expected value / format / purpose of the testname documented anywhere?

This comment has been minimized.

@brahmaroutu

brahmaroutu Jun 28, 2018

Author Contributor

This test name here is the header in the conformance document for the test and format is a readable textual string.

@@ -153,6 +153,14 @@ var _ = SIGDescribe("ServiceAccounts", func() {
}
})

/*
Release: v1.9

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

Why 1.9? What's the significance of this field?

This comment has been minimized.

@brahmaroutu

brahmaroutu May 15, 2018

Author Contributor

This field will be used to track the conformance tests by release. First introduced in 1.9 and then updates the test in 1.12, etc suggesting that this test could potentially impact a conformance run in 1.12 release.

This comment has been minimized.

@tallclair

tallclair May 17, 2018

Member

So this is the release the conformance test was introduced in? (As opposed to when the feature was introduced?)

This comment has been minimized.

@brahmaroutu

brahmaroutu Jun 28, 2018

Author Contributor

yes, intention is to track conformance.

Release: v1.9
Testname: Service Account Tokens Must AutoMount
Description: Ensure that Service Account keys are mounted into the Pod. Pod contains
three containers each will read Service Account Key, root CA Key and Namespace Key respectively from

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

remove Key - that's just an artifact of code reuse: ... containers will each read the Service Account token, root CA, and default namespace

This comment has been minimized.

@brahmaroutu

brahmaroutu Jul 5, 2018

Author Contributor

fixed.

Description: Ensure that Service Account keys are mounted into the Pod. Pod contains
three containers each will read Service Account Key, root CA Key and Namespace Key respectively from
the default API Token Mount path. All these three files MUST exist and the Service Account mount
path MUST be auto mounted to the Pod.

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

nit: s/Pod/Container/

This comment has been minimized.

@brahmaroutu

brahmaroutu Jul 5, 2018

Author Contributor

fixed.

Testname: Service account tokens auto mount optionally
Description: Ensure that Service Account keys are mounted into the Pod only
when AutoMountServiceToken is not set to false. We test the following scenarios here.
1. Create Pod, Pod Spec has AutomountServiceAccountToken set to nil

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

Fix spacing, same below.

This comment has been minimized.

@tallclair

tallclair Jun 8, 2018

Member

still not fixed (extra space before set)

This comment has been minimized.

@brahmaroutu

brahmaroutu Jul 5, 2018

Author Contributor

fixed

Description: Ensure that Service Account keys are mounted into the Pod only
when AutoMountServiceToken is not set to false. We test the following scenarios here.
1. Create Pod, Pod Spec has AutomountServiceAccountToken set to nil
a) Service Account is default,

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

What does this mean?

This comment has been minimized.

@brahmaroutu

brahmaroutu May 15, 2018

Author Contributor

I meant no changes to out-of-the box behavior without configuration.

when AutoMountServiceToken is not set to false. We test the following scenarios here.
1. Create Pod, Pod Spec has AutomountServiceAccountToken set to nil
a) Service Account is default,
b) Service Account is an object with AutomountServiceAccountToken set to true,

This comment has been minimized.

@tallclair

tallclair May 10, 2018

Member

What does "Service Account is an object" mean?

This comment has been minimized.

@brahmaroutu

brahmaroutu May 15, 2018

Author Contributor

I reworded, please verify.

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from f1f3700 to b30c8a4 May 15, 2018

@brahmaroutu

This comment has been minimized.

Copy link
Contributor Author

brahmaroutu commented May 25, 2018

@tallclair Can you please review.

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from b30c8a4 to 04328ef Jun 3, 2018

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from 04328ef to 7a0bca8 Jun 18, 2018

@brahmaroutu

This comment has been minimized.

Copy link
Contributor Author

brahmaroutu commented Jun 27, 2018

@tallclair can you please review.

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from 7a0bca8 to eaa13ae Jun 28, 2018

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Jul 3, 2018

You have still not addressed the comments from my last review. Please address (or respond to) all comments before requesting another review.

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from eaa13ae to ccc31bc Jul 5, 2018

@brahmaroutu brahmaroutu force-pushed the brahmaroutu:conf_serviceaccount branch from ccc31bc to dbeb16c Jul 5, 2018

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Jul 6, 2018

/lgtm
/release-note-none

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 6, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brahmaroutu, tallclair

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 8, 2018

Automatic merge from submit-queue (batch tested with PRs 65882, 65896, 65755, 60549, 65927). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit a936caf into kubernetes:master Jul 8, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation brahmaroutu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@brahmaroutu brahmaroutu referenced this pull request Sep 5, 2018

Closed

REQUEST: New membership for brahmaroutu #69

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.