Fixes AppendManifest used to read files from kodata #428
Conversation
tekton-robot
added
release-note
|
The following is the coverage report on the affected files.
|
sm43
force-pushed
the
fix-append-manifest
branch
from
9e05f6d
to
94835c9
Compare
|
The following is the coverage report on the affected files.
|
| apiVersion: triggers.tekton.dev/v1alpha1 | ||
| kind: ClusterTriggerBinding | ||
| metadata: | ||
| name: bitbucket-pullreq-0.0.2-copy |
There was a problem hiding this comment.
why duplicated file ?
There was a problem hiding this comment.
I was testing having multiple files in the directory
I think this should be solved by restructuring the file tree. This patch seems to be a bit too restrictive, because current way of doing will be the general expectation. so if a data root has 2 separate things which are handled by 2 entities. let us change that to note: main-resources or any other significant name. |
|
/hold |
tekton-robot
added
the
do-not-merge/hold
it turns out my assumption is wrong here. This pr is actually trying to deduplicate Appending of manifest as manifestival doesnot check for duplicates. so let us either move the changes in this patch into a function like (skipDir) and make the need for this change visible through a comment. The flow of logic in the for loop body will be simplified. |
sm43
force-pushed
the
fix-append-manifest
branch
from
94835c9
to
e3e4b24
Compare
Previously, if we pass a path to AppendManifest
for ex. `/kodata/tekton/abc.yaml` and later
`/kodata/tekton/`
both use to read the file and add in the manifest, which
would make manifest to have the same resource twice.
This patch updates the AppendManifest func to use mf.ManifestFrom
which will recursively read all files from a location by passing
the path to it.
Signed-off-by: Shivam Mukhade <[email protected]>
|
The following is the coverage report on the affected files.
|
sm43
force-pushed
the
fix-append-manifest
branch
from
e3e4b24
to
11f7b0e
Compare
| m, err := mf.ManifestFrom(mf.Recursive(yamlLocation)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for i := range files { | ||
| m, err := Fetch(files[i]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| *manifest = manifest.Append(m) | ||
| } | ||
| *manifest = manifest.Append(m) |
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nikhil-thomas 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 |
tekton-robot
added
the
approved
|
The following is the coverage report on the affected files.
|
|
/lgtm |
Previously, if we pass a path to AppendManifest
for ex.
/kodata/tekton/abc.yamland later/kodata/tekton/both use to read the file and add in the manifest, which
would make manifest to have the same resource twice.
This patch updates the AppendManifest func to use mf.ManifestFrom
which will recursively read all files from a location by passing
the path to it.
Closes #425
Signed-off-by: Shivam Mukhade [email protected]
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes