Add initial helloworld integration test #98

bobcatfish · 2018-10-05T00:24:09Z

This is a WIP of adding an integration test to cover #59, which is a
simple TaskRun which has no inputs or outputs but just runs a container
that echoes helloworld.

At this point we have the logic to:

Create the Task and TaskRun
Wait for the TaskRun to have a condition (TODO: actually check the
conditions, waiting to rebase onto add taskrun controller implementation #86)
Get the Build associated (not yet created, will be via add taskrun controller implementation #86)
Get the Pod for the build
TODO: get the logs for that Pod (we've been looking at
https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6
for inspiration)

This also changes namespaces to be test specific, so each test can
create a namespace and tear it down, without needing to keep track of
resources created and delete them individually.

Also removes openAPIV3Schema validation:

When we were using kubebuilder, it generated openAPIV3schema validation
for us in our CRD declarations. However:

No other knative projects use this
Continuing to use this is forcing us to diverge from the other
Knative projects, specifically I found that Status fields, although
optional, were being required by the validation (as described at
CRD validation doesn't accept empty values for type "object" fields kubernetes/apiextensions-apiserver#25), and
the best way to work around it seemed to be to make them nullable (as
per
https://stackoverflow.com/questions/18088294/how-to-not-marshal-an-empty-struct-into-json-with-go)
i.e. pointers, which is different from how Knative works, and
introduces new problems (such as handling when these fields are null)
We (or at least @pivotal-nader-ziada and @shashwathi for sure!) were
maintaining these fiels manually :O

knative-prow-robot · 2018-10-05T00:24:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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:

~~OWNERS~~ [bobcatfish]

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

vdemeester

input_resource_test.go needs some update 👼
Otherwise looks good 👍

nader-ziada

you changed some types to pointers, so need to update the setup of the tests in input_resource_test.go

nader-ziada · 2018-10-05T15:06:50Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

@@ -125,10 +124,9 @@ type TaskRun struct {
 	metav1.ObjectMeta `json:"metadata,omitempty"`

 	// +optional
-	Spec TaskRunSpec `json:"spec,omitempty"`
+	Spec *TaskRunSpec `json:"spec,omitempty"`


why are we changing these to pointers?

These became pointers b/c json marshalling will cause fields to be required, even if they are marked optional, unless they are nullable (https://stackoverflow.com/questions/18088294/how-to-not-marshal-an-empty-struct-into-json-with-go)

(Let me know if you know of a better way to handle this tho!)

Actually though, now that I think about it I don't know why the spec would be optional, so maybe I'll undo this change and make this required instead 🤔

looks like the spec is optional for all the knative/serving types too so maybe ill leave it as optional 🤔🤔🤔

nader-ziada · 2018-10-05T15:08:35Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

 	// +optional
-	//TODO(aaron-prindle) change back to TaskRunStatus
-	Status buildv1alpha1.BuildStatus `json:"status,omitempty"`
+	Status *TaskRunStatus `json:"status,omitempty"`


Also why pointer, in this example in serving its not used as a pointer to make sure its initialized to a value. Otherwise there will be no initial status and other checks could blow up

Interesting! that's a good point, I'll try to dig a bit more and see what's going on

I think I got a bit confused about json + nullable fields and conflated the problems:

If you mark a field as optional but it isn't nullable, it will still be present in generated json, but all the fields will be empty <-- this was unrelated to the problem I was seeing after all! 😅

If the field is required but not present, it will be populated with default values; if the values inside this required field are not optional, then there is a problem b/c they have to be present, so in the serving example you linked the Status fields are all optional, same with Route

I think the real problem was that the fields in Status were required, so I'll make those optional instead.

Thanks for following up on this @pivotal-nader-ziada !!

Thanks for fixing it

Looking into it a bit more it seems the situation is actually slightly worse!

knative/serving isn't using openAPIV3Schema validation but we are ... it looks like this is why I'm running into errors even though this field is not required: kubernetes/apiextensions-apiserver#25

For now the best workaround I can think of is to make these pointers so they can be null - but I think I should open an issue to investigate this further :S

(Open to other ideas too!)

I think if serving isn't using openapi validation, we shouldn't either (build doesn't either). ISTR there were issues with getting it to work with our controllers, possible exactly the issues you're encountering now.

If it's annotation-based validation you're after, you might want knative/pkg#37

Personally I tend to prefer just writing code myself with tests, but 🤷‍♂️

okay discussed with @imjasonh and our current plan (can be changed tho!) is to REMOVE the openapi validation for now 😅 since:

knative isn't using it and it seems like we have to diverge from serving to keep reasonably using it

we have to update it by hand (which @pivotal-nader-ziada and @shashwathi have already been doing! thank you!!)

consistency across Knative repos is better than doing it like we are now

tejal29 · 2018-10-05T18:59:24Z

test/taskrun_test.go

+
+	pods := c.KubeClient.Kube.CoreV1().Pods(namespace)
+
+	fmt.Printf("I GOT PODS %s %s\n", podName, pods)


is there a mapping between build to pod? we can look for pods starting with expected name and error out if they don't

yep! afaik in @aaron-prindle 's implementation, the names of the Builds are the same as the names of the TaskRuns. here we are using that to get the Build: b, err := c.BuildClient.Get(hwTaskRunName... (I'll add a comment about this), and then the Cluster is an attribute on Build which we can use to find the Pod - the next challenge is finding the right containers 😩

The build pods are terminated when the build is done

hmm but even now it's possible to get logs from the pods the Build ran on, even if the Build has completed right? (is it the case that it's a race?)

You can’t get build logs after the build is done unless you have been saving them somewhere else

imjasonh · 2018-10-05T19:03:09Z

test/README.md

+
+```go
+var revisionName string
+err := test.CheckConfigurationState(clients.ServingClient, configName, func(c *v1alpha1.Configuration) (bool, error) {


ConfigurationState doesn't belong in this repo

lol whoa whoops, totally didn't copy paste that in 😅

imjasonh · 2018-10-05T19:06:45Z

test/taskrun_test.go

+	defer tearDown(logger, c.KubeClient, namespace)
+
+	// Create task
+	_, err := c.TaskClient.Create(getHelloWorldTask(namespace))


In the build repo I'm a total pain about preferring the form where it's clearer that err doesn't escape this scope so you don't have to think about whether it's referenced later:

if _, err := doStuff(); err != nil { // handle } // err isn't defined out here

I find it's helpful when reading to be able to mentally drop err from the list of things I care about.

When Go2 has better error handling these'll all just become check calls.

kk ill try to use this when i can! thanks for the pointer. @tejal29 was pointing this out to me as well when we were pairing but i keep forgetting, plz let me know if you see me miss it again!

(The only thing I don't like is when this syntax sometimes forces you to declare var err error)

bobcatfish · 2018-10-05T19:47:42Z

input_resource_test.go needs some update 👼
you changed some types to pointers, so need to update the setup of the tests in input_resource_test.go

Nice, I didn't even realize we had these tests!! <3

@shashwathi

When we were using kubebuilder, it generated openAPIV3schema validation for us in our CRD declarations. However: 1. No other knative projects use this 2. Continuing to use this is forcing us to diverge from the other Knative projects, specifically I found that `Status` fields, although optional, were being required by the validation (as described at kubernetes/apiextensions-apiserver#25), and the best way to work around it seemed to be to make them nullable (as per https://stackoverflow.com/questions/18088294/how-to-not-marshal-an-empty-struct-into-json-with-go) i.e. pointers, which is different from how Knative works, and introduces new problems (such as handling when these fields are null) 3. We (or at least @pivotal-nader-ziada and @shashwathi for sure!) were maintaining these fiels manually :O

tejal29

/lgtm /hold

tejal29 · 2018-10-05T22:00:00Z

test/taskrun_test.go

+
+	pods := c.KubeClient.Kube.CoreV1().Pods(namespace)
+
+	fmt.Printf("I GOT PODS %s %s\n", podName, pods)


tejal29 · 2018-10-05T22:01:21Z

test/taskrun_test.go

+	pods := c.KubeClient.Kube.CoreV1().Pods(namespace)
+	fmt.Printf("I GOT PODS %s %s\n", podName, pods)
+
+	// TODO: Verify logs from the pod, should output hello world


please remove commented out code. Lets add it back in, in next PR

kk, we can always copy paste it back from https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6

This is a WIP of adding an integration test to cover #59, which is a simple TaskRun which has no inputs or outputs but just runs a container that echoes helloworld. At this point we have the logic to: - Create the Task and TaskRun - Wait for the TaskRun to have a condition (TODO: actually check the conditions, waiting to rebase onto #86) - Get the Build associated (not yet created, will be via #86) - Get the Pod for the build - TODO: get the logs for that Pod (we've been looking at https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6 for inspiration) This also changes namespaces to be test specific, so each test can create a namespace and tear it down, without needing to keep track of resources created and delete them individually. Co-authored-by: Aaron Prindle <[email protected]>

tejal29 · 2018-10-05T22:08:19Z

/lgtm

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2018

knative-prow-robot requested review from dlorenc and imjasonh October 5, 2018 00:24

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2018

vdemeester reviewed Oct 5, 2018

View reviewed changes

nader-ziada suggested changes Oct 5, 2018

View reviewed changes

tejal29 reviewed Oct 5, 2018

View reviewed changes

imjasonh reviewed Oct 5, 2018

View reviewed changes

bobcatfish force-pushed the hello_world_integration_test branch from b1c2915 to bc80e37 Compare October 5, 2018 21:45

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 5, 2018

bobcatfish force-pushed the hello_world_integration_test branch 3 times, most recently from 1e2d24c to 34486c2 Compare October 5, 2018 22:00

tejal29 approved these changes Oct 5, 2018

View reviewed changes

bobcatfish force-pushed the hello_world_integration_test branch from 34486c2 to ab3312a Compare October 5, 2018 22:05

knative-prow-robot assigned tejal29 Oct 5, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2018

knative-prow-robot merged commit e3f4ac4 into tektoncd:master Oct 5, 2018

mchmarny unassigned tejal29 Mar 7, 2019

S-hayeon mentioned this pull request Feb 11, 2022

Fix long CRD tmax-cloud/cicd-operator#332

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial helloworld integration test #98

Add initial helloworld integration test #98

bobcatfish commented Oct 5, 2018 •

edited

Loading

knative-prow-robot commented Oct 5, 2018

vdemeester left a comment

nader-ziada left a comment

nader-ziada Oct 5, 2018

bobcatfish Oct 5, 2018

bobcatfish Oct 5, 2018

nader-ziada Oct 5, 2018

bobcatfish Oct 5, 2018

bobcatfish Oct 5, 2018

nader-ziada Oct 5, 2018

bobcatfish Oct 5, 2018

imjasonh Oct 5, 2018

imjasonh Oct 5, 2018

bobcatfish Oct 5, 2018

nader-ziada Oct 5, 2018 •

edited

Loading

tejal29 Oct 5, 2018

bobcatfish Oct 5, 2018

nader-ziada Oct 5, 2018

tejal29 Oct 5, 2018

bobcatfish Oct 5, 2018

nader-ziada Oct 5, 2018

imjasonh Oct 5, 2018

bobcatfish Oct 5, 2018

imjasonh Oct 5, 2018

bobcatfish Oct 5, 2018

bobcatfish commented Oct 5, 2018

tejal29 left a comment

tejal29 Oct 5, 2018

tejal29 Oct 5, 2018

bobcatfish Oct 5, 2018

tejal29 commented Oct 5, 2018


		pods := c.KubeClient.Kube.CoreV1().Pods(namespace)

		fmt.Printf("I GOT PODS %s %s\n", podName, pods)

Add initial helloworld integration test #98

Add initial helloworld integration test #98

Conversation

bobcatfish commented Oct 5, 2018 • edited Loading

knative-prow-robot commented Oct 5, 2018

vdemeester left a comment

Choose a reason for hiding this comment

nader-ziada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Oct 5, 2018

tejal29 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejal29 commented Oct 5, 2018

bobcatfish commented Oct 5, 2018 •

edited

Loading

nader-ziada Oct 5, 2018 •

edited

Loading