Switch `test/*.go` to using YAML rather than structs #4288

abayer · 2021-10-07T17:18:19Z

Changes

/kind cleanup

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

NONE

abayer · 2021-10-07T17:18:31Z

/assign @pritidesai

tekton-robot · 2021-10-07T17:20:55Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/yaml.go	0.0%	19.2%	19.2

abayer · 2021-10-07T17:24:57Z

/test check-pr-has-kind-label

tekton-robot · 2021-10-07T18:05:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/yaml.go	0.0%	19.2%	19.2

tekton-robot · 2021-10-07T18:08:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/yaml.go	0.0%	19.2%	19.2

abayer · 2021-10-07T18:09:28Z

/remove-kind cleanup
/kind cleanup

abayer · 2021-10-07T18:09:46Z

/kind cleanup

pritidesai · 2021-10-07T18:16:10Z

thanks @abayer

Note that test/pipelinefinally_test.go is largely not converted to YAML,
because it relies heavily on parameterized generation of Tasks andPipelines,
to the point where it would be far more complicated to switch to YAML than to
leave it as is.

Indeed, avoiding repeatability landed us here 🤦‍♀️ I would still prefer going towards YAML route if possible 🙏

abayer · 2021-10-07T18:21:23Z

/test pull-kind-label

abayer · 2021-10-07T18:22:34Z

@pritidesai Yeah, pipelinefinally_test.go is just a fair amount of work to change to not being so parameterized, and I wanted to get the bulk of the work done first, at least. I'll probably refactor it in a followup PR.

tekton-robot · 2021-10-07T18:39:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/yaml.go	0.0%	19.2%	19.2

pritidesai · 2021-10-07T21:54:24Z

test/artifact_bucket_test.go

+  volumes:
+  - name: bucket-secret-volume
+    secret:
+      secretName: bucket-secret


NIT: this will be %s instead of a literal bucket-secret, its a common name defined as a constant so results in the same specifications but since we have it parameterized 😉

Ooops - that was an oversight! I was generating the more complicated YAML via marshaling/editing for parameterization, and missed that one. Fixing now!

tekton-robot · 2021-10-08T12:50:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/yaml.go	0.0%	19.2%	19.2

abayer · 2021-10-08T13:43:19Z

@pritidesai - latest force-push dealt with pipelinefinally_test.go. =)

tekton-robot · 2021-10-08T13:44:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/yaml.go	0.0%	19.2%	19.2

abayer · 2021-10-08T16:53:37Z

Ok, test/v1alpha1/... has been switched now too!

abayer · 2021-10-10T15:15:20Z

/retest

tekton-robot · 2021-10-11T08:57:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

abayer · 2021-10-11T12:27:53Z

/hold
Waiting for @pritidesai to slog through reviewing the giant pile of changes to make sure I didn't do anything too stupid. =)

pritidesai · 2021-10-11T19:58:08Z

test/pipelinerun_test.go

+  params:
+  - name: the.path
+    type: string
+  - name: the.dest


NIT: the param name changed here 🙃 dest to the.dest but the param reference has changed as well on line 191 so we are good.

pritidesai · 2021-10-11T20:48:11Z

test/v1alpha1/artifact_bucket_test.go

+  name: %s
+spec:
+  resources:
+    inputs:


alpha task specification include spec.inputs.resources and spec.outputs.resources, here we have spec.resources.inputs and spec.resources.outputs. All the alpha resources are anyways converted to beta before processing, so having this spec in beta format should work. wdyt?

Everywhere else in v1alpha, I used the alpha syntax, so I'll change this to match.

pritidesai · 2021-10-11T20:50:55Z

test/v1alpha1/artifact_bucket_test.go

+  steps:
+  - command: ['/workspace/hellowrld/newfile']
+    image: ubuntu
+    name: runfile


image is runfile and name is addfile, interesting choices 😄 @abayer do you want to stick to the original values?

These are the original values. =)

pritidesai · 2021-10-11T20:53:10Z

test/v1alpha1/artifact_bucket_test.go

+    taskRef:
+      name: %s
+  - name: runfile
+    from:


this from belongs to resources.inputs[].helloworldgit 🙃

Whoops! Fixed.

pritidesai · 2021-10-11T21:33:06Z

test/v1alpha1/workspace_test.go

+      name: %s
+    workspaces:
+    - name: test
+      workspace: foo


NIT: subpath is missing here, but since its set to an empty string, should be ok to leave it unset.

Yeah, I generally skipped empty string cases like that.

pritidesai · 2021-10-11T21:34:15Z

test/v1alpha1/workspace_test.go

+  workspaces:
+  - name: test
+    description: 'test workspace'
+    mountPath: /workspace/test


mountPath is set to /workspace/test instead of /workspace/test/file here

pritidesai · 2021-10-11T21:35:12Z

test/v1alpha1/workspace_test.go

+      name: %s
+    workspaces:
+    - name: test
+      workspace: foo


subpath not set here, similar to the comment ⬆️ , should be fine to leave it unset.

pritidesai · 2021-10-11T21:39:50Z

test/workspace_test.go

+spec:
+  steps:
+  - image: alpine
+    script: 'echo foo > /workspace/test/file'


the script is set to different statement compared to original script, cat /workspace/test/file

pritidesai · 2021-10-11T21:41:42Z

test/workspace_test.go

+spec:
+  steps:
+  - image: alpine
+    script: 'echo foo > /workspace/test/file'


similar to the comment above, the script is set to a different statement but looks like it should be ok since its not adding any value in the test anyways.

Still, just for consistency, I'll change it.

pritidesai · 2021-10-11T21:46:42Z

thanks @abayer for all the hard work on this, I finally finished reviewing all the changes 😓 I have left a few comments. I doubt any of them are blocking since the tests are running fine unless those changes are not necessary or adding any value in terms of validating the functionality 😜

let me know your thoughts ...

I am ready to merge this 💯

fixes tektoncd#4276 Signed-off-by: Andrew Bayer <[email protected]>

abayer · 2021-10-12T12:33:20Z

/hold cancel

abayer · 2021-10-12T12:33:45Z

@pritidesai Ok, nits addressed, and I've taken it off hold!

pritidesai · 2021-10-12T16:36:36Z

alright, thanks a bunch @abayer once again 🙏

Lets merge this 🎉

/lgtm

pritidesai · 2021-10-12T20:06:48Z

/test pull-tekton-pipeline-alpha-integration-tests

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 7, 2021

tekton-robot requested review from bobcatfish and imjasonh October 7, 2021 17:18

tekton-robot assigned pritidesai Oct 7, 2021

abayer force-pushed the yaml-in-test branch 2 times, most recently from fe002dd to 0ed21eb Compare October 7, 2021 18:04

tekton-robot removed the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Oct 7, 2021

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Oct 7, 2021

abayer force-pushed the yaml-in-test branch from 0ed21eb to 1794f47 Compare October 7, 2021 18:35

pritidesai reviewed Oct 7, 2021

View reviewed changes

abayer force-pushed the yaml-in-test branch from 1794f47 to 50ae2b8 Compare October 8, 2021 12:47

abayer force-pushed the yaml-in-test branch from 50ae2b8 to 5de4265 Compare October 8, 2021 13:42

abayer force-pushed the yaml-in-test branch from 5de4265 to 527a273 Compare October 8, 2021 16:52

vdemeester approved these changes Oct 11, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2021

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 11, 2021

pritidesai reviewed Oct 11, 2021

View reviewed changes

Switch test/*.go to using YAML rather than structs

507058f

fixes tektoncd#4276 Signed-off-by: Andrew Bayer <[email protected]>

abayer force-pushed the yaml-in-test branch from 527a273 to 507058f Compare October 12, 2021 12:32

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2021

tekton-robot merged commit 38b9f26 into tektoncd:main Oct 12, 2021

Switch test/*.go to using YAML rather than structs #4288

Switch test/*.go to using YAML rather than structs #4288

Conversation

abayer commented Oct 7, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

abayer commented Oct 7, 2021

tekton-robot commented Oct 7, 2021

abayer commented Oct 7, 2021

tekton-robot commented Oct 7, 2021

tekton-robot commented Oct 7, 2021

abayer commented Oct 7, 2021

abayer commented Oct 7, 2021

pritidesai commented Oct 7, 2021

abayer commented Oct 7, 2021

abayer commented Oct 7, 2021

tekton-robot commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Oct 8, 2021

abayer commented Oct 8, 2021

tekton-robot commented Oct 8, 2021

abayer commented Oct 8, 2021

abayer commented Oct 10, 2021

tekton-robot commented Oct 11, 2021

abayer commented Oct 11, 2021

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

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

pritidesai commented Oct 11, 2021

abayer commented Oct 12, 2021

abayer commented Oct 12, 2021

pritidesai commented Oct 12, 2021

pritidesai commented Oct 12, 2021

Switch `test/*.go` to using YAML rather than structs #4288

Switch `test/*.go` to using YAML rather than structs #4288

abayer commented Oct 7, 2021 •

edited

Loading