Update openwhisk-knative pipeline to v1beta1 #375

mrutkows · 2020-06-19T15:43:29Z

Changes

This PR is a complete update of the OpenWhisk to Knative pipeline from v1alpha1 level to v1beta1 resource levels. This pipeline also adds conditional logic to support additional "branches" of the pipeline to sequence tasks that can detect and build serverless applications for not only NodeJS (previously the only lang. supported), but also Python and Java. In addition, this pipeline utilizes a workspace for sharing data/state across tasks. Each language branch has its own sample functions with instructions for users to build and test on knative the services they build.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes docs (if user facing)
Commit messages follow commit message best practices
Yaml file complies with yamllint rules.

See the contribution guide
for more details.

linux-foundation-easycla · 2020-06-19T15:43:32Z

The committers are authorized under a signed CLA.

✅ Matt Rutkowski (aa17cc5)

tekton-robot · 2020-06-19T15:43:44Z

Hi @mrutkows. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mrutkows · 2020-06-19T17:07:04Z

@pritidesai please can you give your review?

pritidesai · 2020-06-19T17:12:48Z

/ok-to-test

mrutkows · 2020-06-22T16:05:39Z

The (openwhisk) pipeline and its tasks (that are being updated in this PR) have no integration tests, but now seem to be blocked by some repo.-wide integration tests that are failing? What is the process at this point?

ghost · 2020-06-23T15:32:54Z

@mrutkows you'll need to rebase on top of the latest master to get the fixed integration tests.

chmouel

I haven't reviewed the whole review but the error in the CI seems to point that the test runner create multiple times the same resource which fail :

+ kubectl -n openwhisk-549 create -f /tmp/.mm.0l7pIB
taskrun.tekton.dev/taskrun-install-npm-packages created
taskrun.tekton.dev/taskrun-install-npm-packages-without-path created
Error from server (AlreadyExists): error when creating "/tmp/.mm.0l7pIB": pipelineresources.tekton.dev "app-git" already exists
+ teardown_test_resources

As noted your test should only create one pipelineresorce and have it reference it directly from the Task/Pipeline

chmouel · 2020-06-24T06:43:57Z

openwhisk/.gitignore

@@ -0,0 +1,46 @@
+##############################################


Any chance you can remove this file ? See here for the reasoning why gitignore should be kept local https://julien.danjou.info/properly-managing-your-gitignore/

I am not so much convinced by this argument of having gitignore everywhere in the catalogs, if the gitignore contained some specific files for the specifics openwhisk files then that would be fine, but those are mostly editors/ide exclude, which are best kept in the user configuration

@chmouel the design of gitignore supports and encourages use of directory-specific use (see https://labs.consol.de/development/git/2017/02/22/gitignore.html) to allow finer-grained exclusion. It seems that since the catalog is effectively a collection of subprojects each would want and be encouraged to have its own rules specific to its installation/configuration/tests.

However, I agree with the point from the ref. article and removed the "editor" specific entries in the file. Hopefully, what remains is accurate to the examples only.

What remains are artifacts created, if you follow the "openwhisk" specific readme's optional instructions to manually run the examples the scripts and steps cause many new files to be created which we do not want checked in by developers who may be updating this specific catalog entry.

I am not so much convinced by this argument of having gitignore everywhere in the catalogs

@chmouel As indicated on the update, gitignore is now specific to the openwhisk entry and artifacts that are created from following its README only.

chmouel · 2020-06-24T06:45:54Z

openwhisk/01-dice-color.json

@@ -0,0 +1,5 @@
+{


Those .json files are examples, isnt it ? By convention we movet them to the examples/ directory

@chmouel yes, had thought to move them, but it took so much time to get the instructions working that I had hoped to defer this to a subsequent PR as it seemed the priority was to get the resources themselves updated to v1beta1 level.

If this is a "blocker" I can try to find some time in the next several days to do so, but would it suffice to open an issue and assign it to myself against just this catalog entry?

@chmouel I decided to co-locate the JSON parameter (input) files in the same subdirectory with the Java pipelineRun YAML as this is the only pipelinerun (example) that uses these input files.

openwhisk/tests/taskrun-install-npm-packages.yaml

openwhisk/README.md

pritidesai · 2020-06-25T20:59:31Z

openwhisk/README.md


-## Clone this repository
+The `PipelineRun` resource used to build a Python application is named `build-javascript-app-image` and derived from the template file [pipelinerun-build-padding-app.yaml.tmpl](pipelinerun/javascript/pipelinerun-javascript.yaml.tmpl).


s/python/javascript/

thanks for fixing this 👍

pritidesai · 2020-06-25T21:02:50Z

openwhisk/README.md


-## Create Service Account for our Runtime Builds
+    ```bash
+    tkn pr describe build-javascript-app-image


please replace this with tkn pr describe build-python-app-image

pritidesai · 2020-06-25T21:11:13Z

openwhisk/README.md


-This directory contains Tekton Task which can be used to Build and Serve Knative compatible applications (i.e., OpenWhisk Actions) on Kubernetes.
+*This catalog offering provides a single pipeline that can be used to build either [Apache OpenWhisk](https://openwhisk.apache.org/) or [Knative](https://openwhisk.apache.org/) compatible containers for supported runtimes used to execute OpenWhisk serverless functions.*


please update the knative link here

Thanks @pritidesai. Silly copy/paste mistake. Will take the opportunity to add links a couple of other places in the opening paragraphs that may be helpful for easily accessing context.

pritidesai · 2020-06-25T21:11:32Z

openwhisk/README.md

- [x] Knative v0.5.0
- [x] Tekton Pipeline
- [x] Knative Client (OPTIONAL)
+The project provides a set of [supported language runtimes](https://openwhisk.apache.org/downloads.html#component-releases) that includes a proxy that enforces a *documented contract for function initialization (function injection) and execution* along with a standard context. Several of these runtimes, such as NodeJS, Python and Java, have been updated to support execution on as containers on either OpenWhisk or [Knative](https://openwhisk.apache.org/) clusters. In the latter case, the resultant containers can be run as on Knative without requiring an OpenWhisk control plane.


same as above, please update the knative link here

pritidesai · 2020-06-25T21:13:16Z

thanks @mrutkows for this PR, have left some minor comments, overall it looks great, we might want to address the overall structure, moving samples under examples like @chmouel mentioned 🙏

mrutkows · 2020-07-07T19:59:16Z

@pritidesai Also, just removed demo-commands.sh which was a legacy script to run just the java demo. In the future, we should look to see about creating a java-only pipeline (and perhaps for each of the language branches). It would be great if we could eventually have what Simon has been pushing for which are "pipeline tasks" so we could have a master pipleine that runs each of the language "pipeline branches" as tasks (with switch logic).

mrutkows · 2020-07-09T17:27:37Z

@chmouel Hi Chris, seeing if you wished to re-review post-changes. Thanks!

mrutkows · 2020-07-09T17:28:29Z

@pritidesai Hi Priti, I think that all your comments have been addressed; would you be able to re-review?

pritidesai · 2020-07-09T18:22:58Z

@pritidesai Hi Priti, I think that all your comments have been addressed; would you be able to re-review?

Sorry @mrutkows for delay, will review it today 🙏

pritidesai · 2020-07-10T05:30:48Z

openwhisk/README.md


 ---

-## Troubleshooting
+#### Java


NIT: s/####/###/ like other runtimes NodeJs and Python

pritidesai · 2020-07-10T05:43:08Z

Thanks @mrutkows for updates, overall YAML content and organization looks good to me including directories Tekton resources:

And subdirectories for each runtime:

@chmouel please take a look whenever you get a chance 🙏

tekton-robot · 2020-07-11T21:05:07Z

@mrutkows: PR needs rebase.

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.

afrittoli · 2020-07-14T17:03:18Z

@mrutkows as you rebase, would you mind adding a body to the commit message? (https://github.com/tektoncd/community/blob/master/standards.md#commit-messages) Thanks!

afrittoli · 2020-07-14T17:07:05Z

@vdemeester @sthaha can we discover pipelines and tasks in subfolders with hub?
I think it's really useful to have an E2E example of how to use a task or set of tasks, by I wonder how we can surface this kind of setup in the hub.

sthaha · 2020-07-15T00:03:44Z

@vdemeester @sthaha can we discover pipelines and tasks in subfolders with hub?

Yes and no, we can parse all files in a repo but I think the ReOrg TEP addresses the issue of computable file paths from kind/name/version information i.e. given a resource task that has name openwhisk and version 0.1 where should a tool look for the resource definitions?

In TEP we chose kind/resource/version/resource.yaml as the structure and it requires the current PR to be broken into

  /task/openwhisk/0.1/openwhisk.yaml
  /pipeline/openwhisk/0.1/openswhisk.yaml
  /condition/openwhisk/0.1/openwhisk.yaml

So that the hub can look for tasks under task and pipeline under /pipeline

cc: @mrutkows

I have no (strong) opinion on this structure and is open to changing this structure if it suits us better but let's do it as an amendment to the existing TEP.

I think it's really useful to have an E2E example of how to use a task or set of tasks, by I wonder how we can surface this kind of setup in the hub.

That is a great question and one that we should address especially because with support for pipelines in hub (doesn't exist now) we would have to display/inform users of dependent tasks, conditions etc ...

sthaha · 2020-07-15T00:16:31Z

Thanks @mrutkows for updates, overall YAML content and organization looks good to me including directories Tekton resources:

@mrutkows @pritidesai unfortunately this breaks the organisation proposed in the TEP :( which requires files to be organised as {resource}/{name}/{version}/{name}.yaml

e.g. /pipeline/build-openwhisk-app/0.1/build-openwhisk-app.yaml instead of the current structure task/openwhisk/0.1/pipeline/pipeline-to-build-openwhisk-app.yaml or move all files except /task/openwhisk/0.1/openwhisk.yaml to /task/openwhisk/0.1/samples

Additionally, the files are missing mandatory metadata as stated in https://github.com/tektoncd/community/blob/master/teps/0003-tekton-catalog-organization.md#versioning-resources

viz.

metadata:
   labels:
        app.kubernetes.io/version:
  annotations:
       tekton.dev/pipelines.minVersion: "0.12.1"

spec:
  description: >- 
      one line summary of the resource
  
      Para describing the resource

You may also add additional metadata such as tekton.dev/tags and tekton.dev/displayName see: https://github.com/tektoncd/catalog/blob/master/task/openwhisk/0.1/openwhisk.yaml for example.

mrutkows · 2020-07-15T19:11:50Z

@sthaha Please know that this PR predated the referenced TEP, and I did not place them under the "task" folder structure, but rebased this Monday to adapt to a change done (#416) on behalf of all existing content. I will look at how to conform to the TEP, but we are effectively a pipeline that (hopefully in the future) may be able to run in its entirety as a "pipeline task" (term being used) and fear we are falling under "uncharted ground" in the proposal that was recently implemented.

After reading the TEP, it looks like there is discretion under "pipeline" top-level structure, if I refactor to move our existing sub-directory structure (and README instructions) under:

/pipelines/openwhisk/0.1/
  /conditions
  /images
  /pipelines
  /tasks
  /samples
    /pipelineruns/xxx

Would that meet the criteria in your mind?

I just want to have a v1beta1 level pipeline that the openwhisk community can use as soon as possible as they are stuck on alpha tasks/samples (which are quite dated).

sthaha · 2020-07-16T01:45:06Z

@mrutkows , based on our conversation on Slack, like @afrittoli pointed out the TEP is in "proposed" state, so we can go ahead and merge the PR and then later rearrange the files to conform to the TEP.

Please note that having the mandatory annotations and the spec.description will help others discover the task on hub - https://ui-tekton-hub.apps.openshift-web.p0s5.p1.openshiftapps.com/

You may want to address the comment on slack if you haven't already.

@mrutkows I wonder if it would make sense to keep the shell script, service account and other non-tekton resources under a dedicated folder, since those are not something that will be directly consumable via registry API?

mrutkows · 2020-07-16T17:56:59Z

@sthaha @pritidesai I have concluded that with the recent hub going live that we should indeed adapt towards the referenced TEP. Therefore, the latest commit moves the openwhisk folder (catalog entry) under /pipeline instead of where it had been moved (i.e., /task). Additionally, I moved the pipeline resource YAML to the base directory (renaming it simply openwhisk.yaml)and added the proper tags so that the Hub will have the correct description and search terms (along with min. versions). Finally, updated the resource deployment script deploy.sh.

I look forward to being the first "pipeline" under the new structure and figuring out how we evolve towards the best practices for pipelines and their often many dependencies and considerations beyond that of a single task.

This commit represents a complete update of the OpenWhisk to Knative pipeline from v1alpha1 level to v1beta1 resource levels. This pipeline also adds conditional logic to support additional "branches" of the pipeline to sequence tasks that can detect and build serverless applications for not only NodeJS (previously the only lang. supported), but also Python and Java. In addition, this pipeline utilizes a workspace for sharing data/state across tasks. Each language branch has its own sample functions with instructions for users to build and test on knative the services they build.

mrutkows · 2020-07-23T16:29:49Z

@pritidesai @chmouel @sbwsg @sthaha I would truly appreciate a review in hope that after addressing all comments and rebasing/refactoring/adapting for the draft hub layout proposal that this finally can be merged.

pritidesai · 2020-07-27T22:22:21Z

thanks @mrutkows for the changes

@sthaha please check @mrutkows reorganization, I have verified the content and all the YAML works. I have ran it and tested the pipeline.

tekton-robot · 2020-07-28T09:52:02Z

[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

sthaha · 2020-07-28T11:32:43Z

@vdemeester @mrutkows Just noting that I am okay with merging the patch since the TEP is still in proposed but please be aware that the way it is currently organised doesn't conform to proposal (see example) and wouldn't show up in the Hub preview.
This can be addressed in a separate patch of course.

I am also unsure about the .tmpl files.

Since the pipeline is usable, I don't think we should bikeshed on it. However, we should discuss this further in our WG call.

cc: @chmouel

chmouel · 2020-07-30T06:30:47Z

/lgtm

thanks for filling #453 @sthaha 🙇

Looking Good! 🤙🏽

Fix catalog ci failure

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2020

tekton-robot requested review from dlorenc and a user June 19, 2020 15:43

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 19, 2020

mrutkows changed the title ~~Update openwhisk-jnative pipeline to v1beta1~~ Update openwhisk-knative pipeline to v1beta1 Jun 19, 2020

mrutkows marked this pull request as ready for review June 19, 2020 17:05

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 19, 2020

dibyom mentioned this pull request Jun 22, 2020

Catalog Integration Tests are failing #379

Closed

chmouel requested changes Jun 24, 2020

View reviewed changes

pritidesai reviewed Jun 25, 2020

View reviewed changes

openwhisk/README.md Outdated Show resolved Hide resolved

pritidesai reviewed Jun 25, 2020

View reviewed changes

pritidesai reviewed Jul 10, 2020

View reviewed changes

openwhisk/README.md Outdated

---

## Troubleshooting

#### Java

Copy link

Member

pritidesai Jul 10, 2020

Choose a reason for hiding this comment

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

NIT: s/####/###/ like other runtimes NodeJs and Python

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2020

afrittoli removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020

vdemeester approved these changes Jul 28, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2020

sthaha mentioned this pull request Jul 29, 2020

Reorganise openwhisk to conform with catalog re-org TEP #453

Closed

tekton-robot assigned chmouel Jul 30, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

tekton-robot merged commit a9cdd92 into tektoncd:master Jul 30, 2020

mrutkows deleted the openwhisk-beta branch July 30, 2020 17:13

rupalibehera pushed a commit to rupalibehera/catalog that referenced this pull request Nov 11, 2021

Merge pull request tektoncd#375 from savitaashture/ci-failure-catlog-fix

595c208

Fix catalog ci failure

		@@ -0,0 +1,46 @@
		##############################################


		## Clone this repository
		The `PipelineRun` resource used to build a Python application is named `build-javascript-app-image` and derived from the template file [pipelinerun-build-padding-app.yaml.tmpl](pipelinerun/javascript/pipelinerun-javascript.yaml.tmpl).


		This directory contains Tekton Task which can be used to Build and Serve Knative compatible applications (i.e., OpenWhisk Actions) on Kubernetes.
		This catalog offering provides a single pipeline that can be used to build either [Apache OpenWhisk](https://openwhisk.apache.org/) or [Knative](https://openwhisk.apache.org/) compatible containers for supported runtimes used to execute OpenWhisk serverless functions.

Update openwhisk-knative pipeline to v1beta1 #375

Update openwhisk-knative pipeline to v1beta1 #375

Conversation

mrutkows commented Jun 19, 2020

Changes

Submitter Checklist

linux-foundation-easycla bot commented Jun 19, 2020 • edited Loading

tekton-robot commented Jun 19, 2020

mrutkows commented Jun 19, 2020

pritidesai commented Jun 19, 2020

mrutkows commented Jun 22, 2020 • edited Loading

ghost commented Jun 23, 2020

chmouel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrutkows Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

mrutkows Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrutkows Jun 24, 2020 • 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

pritidesai commented Jun 25, 2020

mrutkows commented Jul 7, 2020

mrutkows commented Jul 9, 2020

mrutkows commented Jul 9, 2020

pritidesai commented Jul 9, 2020

Choose a reason for hiding this comment

pritidesai commented Jul 10, 2020

tekton-robot commented Jul 11, 2020

afrittoli commented Jul 14, 2020

afrittoli commented Jul 14, 2020

sthaha commented Jul 15, 2020

sthaha commented Jul 15, 2020 • edited Loading

mrutkows commented Jul 15, 2020 • edited Loading

sthaha commented Jul 16, 2020

mrutkows commented Jul 16, 2020 • edited Loading

mrutkows commented Jul 23, 2020

pritidesai commented Jul 27, 2020

tekton-robot commented Jul 28, 2020

sthaha commented Jul 28, 2020

chmouel commented Jul 30, 2020

linux-foundation-easycla bot commented Jun 19, 2020 •

edited

Loading

mrutkows commented Jun 22, 2020 •

edited

Loading

mrutkows Jun 24, 2020 •

edited

Loading

mrutkows Jun 24, 2020 •

edited

Loading

mrutkows Jun 24, 2020 •

edited

Loading

sthaha commented Jul 15, 2020 •

edited

Loading

mrutkows commented Jul 15, 2020 •

edited

Loading

mrutkows commented Jul 16, 2020 •

edited

Loading