-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[TEP074] Remove Storage pipelineResources #6014
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 |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
ab4bd05
to
00b8456
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
00b8456
to
2f142e1
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
@@ -948,7 +937,7 @@ func (c *Reconciler) createTaskRun(ctx context.Context, taskRunName string, para | |||
tr.Annotations[workspace.AnnotationAffinityAssistantName] = getAffinityAssistantName(pipelinePVCWorkspaceName, pr.Name) | |||
} | |||
|
|||
resources.WrapSteps(&tr.Spec, rpt.PipelineTask, rpt.ResolvedTaskResources.Inputs, rpt.ResolvedTaskResources.Outputs, storageBasePath) | |||
resources.WrapSteps(&tr.Spec, rpt.PipelineTask, rpt.ResolvedTaskResources.Inputs, rpt.ResolvedTaskResources.Outputs) |
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.
Since removing the artifacts/pvc-bucket would worth to clean up in a separate PR that before the removal of storage resource as the path of the output/input resources, I have been trying to come up with a better way for this removal:
For the WrapSteps
, I would like to check if it is w to remove this in this PR or if it is worth changing the corresponding test cases while it is going to be removed eventually?
But if so, it will be related with removing/ cleaning up the pipeline/pkg/reconciler/pipelinerun/resources/input_output_steps.go
🤔
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.
@JeromeJu are you still having trouble with this bit? I think the git pipelineresource also uses artifact storage; it might be easier to not delete artifact storage in this PR or to delete the git resource and the storage resource in the same PR
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.
Thanks @lbernick , that makes sense. Since Git and Storage pipelineresources both use artifact storage, we would have to remove them together to avoid the situation where we are introducing changes of Git resources removal in this Storage removal PR along.
Therefore, I tried to merge the changes and restore the parts on Git resources to be restored for image type at #6150.
And if that looks good I will try to make Image Resources removal as the last PR along with the removal of the generic functions as the removal of both Storage and Git resources has already been a large chunk.
2f142e1
to
9d37509
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
9d37509
to
dfce881
Compare
The following is the coverage report on the affected files.
|
dfce881
to
19d74e7
Compare
52dec61
to
2bc8ab5
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
2bc8ab5
to
2e310bd
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
2e310bd
to
3d50952
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/retest |
3d50952
to
a1f117f
Compare
This commit removes the Storage Resources support. This PR removes `github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1/storage`, `pkg/artifacts` and the respective test cases.
a1f117f
to
193d46a
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
This PR is ready for review, thanks! PTAL /assign @afrittoli @lbernick |
/assign @Yongxuanzhang |
| `name` | The name of the resource. | | ||
| `type` | Type value of `"gcs"`. | | ||
| `location` | The fully qualified address of the blob storage. | | ||
|
||
#### Variables for the `Cluster` type |
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.
it looks like there's some cloudevent and cluster docs hanging around here
@@ -948,7 +937,7 @@ func (c *Reconciler) createTaskRun(ctx context.Context, taskRunName string, para | |||
tr.Annotations[workspace.AnnotationAffinityAssistantName] = getAffinityAssistantName(pipelinePVCWorkspaceName, pr.Name) | |||
} | |||
|
|||
resources.WrapSteps(&tr.Spec, rpt.PipelineTask, rpt.ResolvedTaskResources.Inputs, rpt.ResolvedTaskResources.Outputs, storageBasePath) | |||
resources.WrapSteps(&tr.Spec, rpt.PipelineTask, rpt.ResolvedTaskResources.Inputs, rpt.ResolvedTaskResources.Outputs) |
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.
@JeromeJu are you still having trouble with this bit? I think the git pipelineresource also uses artifact storage; it might be easier to not delete artifact storage in this PR or to delete the git resource and the storage resource in the same PR
@@ -1204,3 +1205,30 @@ func tearDownV1Beta1CustomTask(ctx context.Context, t *testing.T, c *clients, na | |||
|
|||
tearDown(ctx, t, c, namespace) | |||
} | |||
|
|||
// updateConfigMap updates the config map for specified @name with values. We can't use the one from knativetest because |
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.
was this moved from somewhere else or just a weird rebase?
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.
It used to be in the deleted artifact_bucekt_test.go
and referenced here.
@@ -64,13 +64,6 @@ metadata: | |||
name: %s | |||
namespace: %s | |||
spec: |
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.
seems like the changes in this file are mainly related to git resources
@@ -55,6 +55,7 @@ const ( | |||
scmRemoteUserPassword = "ab_d1234HIJKL" | |||
// Defined in git-resolver/gitea.yaml's "gitea" StatefulSet, in the env for the "configure-gitea" init container | |||
scmGiteaAdminPassword = "giteaPassword1234" | |||
systemNamespace = "tekton-pipelines" |
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.
what's the reason for this change?
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.
It is moved from the deleted artifact_bucket_test.go
Closing as #6150 supersedes this PR |
Changes
Remove Storage PipelineResources
This commit removes the Storage Resources support.
This PR removes
github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1/storage
,pkg/artifacts
and the respective test cases.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes