-
Notifications
You must be signed in to change notification settings - Fork 577
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
catlin-validate task to lint task yaml's or set of task yamls #1178
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @pratiktest. 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 Once the patch is verified, the new status will be reflected by the 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. |
Catlin Output
Catlin script lint Output
|
|
Catlin Output
Catlin script lint Output
|
@pratiktest can you please sign the CLA? |
description: This workspace is where catlin output will be stored | ||
steps: | ||
- name: catlin | ||
image: docker.io/pratikkale/catlin:v1@sha256:794ae5b2e3a22ebd481572769b552d924c48dd411fa1cf82145abf6ad258aad5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have image of catlin available at gcr gcr.io/tekton-releases/dogfooding/catlin
, can we use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinamra28 Changed task to use catlin image from gcr
Catlin Output
Catlin script lint Output
|
Catlin Output
Catlin script lint Output
|
Catlin Output
Catlin script lint Output
|
Catlin Output
Catlin script lint Output
|
Catlin Output
Catlin script lint Output
|
Catlin Output
Catlin script lint Output
|
@vinamra28 I am working on getting approval from my company manager for signing CLA, meanwhile I fixed all the shell checks and catlin validate checks, however the catlin output posted in the comments on my PR is showing failed shell checks for other tasks which are not part of this PR |
@vinamra28 I have signed CLA, can you please help review again. Thanks!! |
@vinamra28 @bobcatfish Do i need ok-to-test label for running the other checks? |
/ok-to-test |
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove these extra lines 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,8 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
- name: inputFile | ||
description: | | ||
File that contains catlin input. This file contains relative paths to the task yaml's delimited by newline | ||
this file will be read from /workspaces/catlin-input/$(params.inputFile) and catlin validate will be applied to each file | ||
example if file contains below paths (as you can see the paths are separated by newlines) | ||
task/git-clone/0.9/git-clone.yaml | ||
task/git-clone/0.1/git-comment.yaml | ||
it will run catlin on the above paths in the source workspace one after another and store its output in catlin-output workspace | ||
final command run will be | ||
'catlin validate $(workspaces.source.path)/task/git-clone/0.9/git-clone.yaml >> $(workspaces.catlin-output.path)/output.txt' | ||
'catlin validate $(workspaces.source.path)/task/git-clone/0.1/git-comment.yaml >> $(workspaces.catlin-output.path)/output.txt' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can have this parameter of type array and pass all the files path directly. Mounting a separate workspace would have been useful if we were mounting tasks there. Let me know your thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A file in a workspaces, I feel is useful for task to task communication. I Prefer a file in a workspace which will contain the input over an array parameter since it scales better for larger inputs when one task needs to pass data to another task . As catlin-validate task can accept multiple inputs, the input paths can be long.
Imagine another task that generates the paths on which we need to run catlin-validate
If I use array params, then this other task needs to use results to pass on to the input array parameter. Since catlin-validate accepts multiple input files and this can be long they can exceed kubernetes termination string
Instead If the other task uses a PVC and an input-file in the PVC we do not have any such restrictions and this same PVC can be passed on as a workspace to catlin-validate task , which makes the task work for large inputs
Let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, due to affinity-assistance
, we won't be able to mount more than 1 pvc at a time (@vdemeester please correct me if I am wrong 😅), if the former is true then before starting the pipeline, we'll have to mount a ConfigMap containing the input file. I guess this approach can restrict the users who would like to first find the tasks they want to validate using catlin and then pass the list to this task? One solution to this can be binding one pvc workspace with both inputFile
and source
workspace and then go ahead but if this is the idea then can't we just have one workspace source
in the task where we check for the input file ? This goes for catlin-output
workspace as well.
@vdemeester @piyush-garg please let me know if there is some gap from my side 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinamra28 The PVC for source and for catlin-input ideally should be different PVC's as they are for completely different purpose. I have created tasks where I have bounded multiple PVC's into different workspaces for a task.
Irrespective I think both these workspaces are readOnly so even mounting the same PVC for these workspaces should work, it is completely upto the user. For this task I think I should mark these workspaces as readOnly as nothing is written to these workspaces and they are merely inputs to catlin-validate task
we won't be able to mount more than 1 pvc at a time
Why would an affinity assistance restrict mounting more than 1 PVC at a time?
For instance an example pipeline would be
Pipeline ---> Task1 -> Task2 -> catlin-validate
Task1 -> clones takton catalog in PVC1 workspace
|
|
Task2 -> Gets all the task paths on which we need to run catlin-validate and creates input.txt file in PVC2
|
|
Task3 (catlin-validate)
-> Loads source (tekton catalog source code with tasks folder) from PVC1
-> Loads catlin-input (task paths) from PVC2
For above since tasks share same workspace backed by PVC the affinity assistant will probably schedule the pods executing the task on the same Kubernetes Node, but it should not restrict mounting more than 1 PVC at a time. Let me know if my understanding is correct. I am also happy to write a test case to validate this
The reason for separating workspaces out is isolation of concern.
Source workspace refers to the source code which contains all the tekton-catalog tasks (for instance source can contain the git clone of tekton catalog or your internal company tekton catalog)
catlin-input workspace contains the input of paths of task files on which we want to run catlin-validate.
Both these workspaces can be backed with PVC's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinamra28 any suggestions comments on above?
task/hello/0.1/hello.yaml | ||
task/bye/0.1/bye.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these can be passed using parameters of type array. Can you please explain the exact usecase of mounting a configmap for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used a configmap since the input to the task is a file in a workspace instead of array params. This (#1178 (comment)) is the reason to keep it a file in a workspace rather than input parameter array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename this file to something meaningful? like source-multiple-tasks-configmap.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly other files can be renamed. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the taskrun is going to fail, CI is going to fail. Probably you can add this in samples directory and refer in README for others to consume
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, Wonder how we can test negative use cases where a task is supposed to fail on certain conditions to not let the pipeline proceed. This test case would be important for as an example as in this case we would want to test that pipeline indeed fails and stops if catlin lint fails to prevent an accidental publish
I have moved The error case in samples folder and updated the README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, Wonder how we can test negative use cases where a task is supposed to fail on certain conditions to not let the pipeline proceed. This test case would be important for as an example as in this case we would want to test that pipeline indeed fails and stops if catlin lint fails to prevent an accidental publish I have moved The error case in samples folder and updated the README.md
yeah this is one limitation with out current infrastructure 😓 Also we are planning to move away from this community repository and moving towards decentralised one so this repository is going to be archived. We are yet to come with timelines but yes, that's the long term idea.
Catlin Output
Catlin script lint Output
|
1928ef4
to
c3a744d
Compare
Catlin Output
Catlin script lint Output
|
1 similar comment
Catlin Output
Catlin script lint Output
|
Catlin Output
Catlin script lint Output
|
Catlin Output
Catlin script lint Output
|
/retest |
Description 1.Rename files 2.Move Error test cases to samples folder 3.Remove unecessary spaces
Catlin Output
Catlin script lint Output
|
@vinamra28 Thank you for the detailed review!!, all checks have now passed |
@vinamra28 @bobcatfish Anything else is needed for the task to be merged? |
Issues go stale after 90d of inactivity. /lifecycle stale Send feedback to tektoncd/plumbing. |
Changes
Add a new tekton task that will run catlin validate command for a task yaml or set of tasks yamls
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
contains
/kind <type>
. Valid types are bug, cleanup, design, documentation,feature, flake, misc, question, tep
File path follows
<kind>/<name>/<version>/name.yaml
Has
README.md
at<kind>/<name>/<version>/README.md
Has mandatory
metadata.labels
-app.kubernetes.io/version
the same as the<version>
of the resourceHas mandatory
metadata.annotations
tekton.dev/pipelines.minVersion
mandatory
spec.description
follows the conventionSee the contribution guide for more details.