-
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
cleanup to resource types #73
cleanup to resource types #73
Conversation
/assign @bobcatfish |
f035925
to
6cfda11
Compare
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 did an initial quick pass, mostly wondering if some of the references are not quite sync-ed up properly (it's super hard to keep in sync since there is literally no way to test!!).
I like the new names!
@@ -1,4 +1,4 @@ | |||
apiVersion: pipeline.knative.dev/v1beta1 | |||
apiVersion: pipeline.knative.dev/v1alpha1 |
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.
whoops, can't believe how easy it is to miss these! Thanks for catching it :D
|
||
// PipelineResourceTypeImage indicates that this source is a docker Image. | ||
PipelineResourceTypeImage PipelineResourceType = "image" | ||
) |
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.
niiiiiice!
@@ -5,48 +5,57 @@ metadata: | |||
namespace: default | |||
spec: | |||
tasks: |
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 for updating this! sorry for the extra confusion of having two sets of having two sets of examples 🤞 #20 🤞
samples/pipeline_v1alpha1_task.yaml
Outdated
resources: | ||
- resourceRef: | ||
name: registry | ||
name: builtImage # registy is the name of the ArtifactStore |
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 think this comment is no longer relevant
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.
will remove it
namespace: default | ||
spec: | ||
name: stagingRegistry |
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 bunch of the image resources have names like registry
or stagingRegistry
, which I find a bit confusing b/c the info here is about a particular image in a registry, not just a registry (correct me if I'm wrong!)
could we give them names that reflect that they are images?
also i've been assuming that the name
field here is name of the image, and that the url
below would be the url to the registry, so that one could use them separately (or maybe we should change url
to registryUrl
?)
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.
correct assumption, name is for the image and url is for the registry, i'll update the name and you can decide after if you want to the url to registryUrl, but I think its implied
value: githubServiceAccount | ||
--- | ||
apiVersion: pipeline.knative.dev/v1alpha1 | ||
kind: Resource |
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! so one would need to reference each Resource
separately?
I am fine with doing this but I assumed differently in the examples I wrote out, e.g. in #64 I assumed you could declare one Resource
type which contained a list of all the resources you needed for your Pipeline. One thing I like about that is that it means the number of yaml files a person would need to interact with their Pipeline is limited.
But if it makes more sense to have a separate object for each Resource
we can update the issues!
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 made this change as part of this PR actually, its because when I started working on the git resource I found that having an array in the Spec that refers to another object makes the code too awkward
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.
okay sounds good!
namespace: default | ||
spec: | ||
name: stagingRegistry-resource |
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.
do we need both the name in the spec and the name in the metadata?
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.
not sure if we need both?
kind: Resource | ||
metadata: | ||
name: kritis-resources-image |
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.
some of the resources in these examples I don't see referenced anywhere, maybe the references in the Pipelines need to be updated?
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'll check them again, might have missed it
@bobcatfish addressed comments |
examples/pipelines/guestbook.yaml
Outdated
- name: registry | ||
key: stagingRegistry | ||
- name: redisImage | ||
key: stagingImage |
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 think something is a bit odd here, so this seems to be saying:
- The Task
build-push
has an output calledredisImage
- For that output, use the Resource called
stagingImage
Looking at the Task:
It looks like the image is actually called builtImage
, so I think this section should be:
outputSourceBindings:
- name: builtImage
key: stagingImage
samples/pipeline_v1alpha1_task.yaml
Outdated
resources: | ||
- resourceRef: | ||
name: registry | ||
name: builtImage |
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'm confused why we need a resourceRef
here and what it would be referencing
We're hooking up all the refs in the Pipeline
, and the Task
doesn't know what Resource
it will be running against until it is supplied.
Can you explain more about why this is needed here? (maybe it isn't?)
storeKey: registry # registy is the name of the ArtifactStore | ||
- name: builtImage | ||
type: image | ||
storeKey: registry # registy is the name of the ArtifactStore |
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 think this storeKey
is out of date - in fact i think what we want here is just a reference to the Resource
that we actually used
- rename `StandardResource` to `PipelineResource` - rename `ResourceSuper` to `PipelineResourceInterface` and make func public - change to PipelineResourceSpec to not be an array
remove resources from pipeline main yaml add resourceRef to source binding remove unused name from ResourceSpec update examples
b499a94
to
2145e1e
Compare
@bobcatfish addressed the comments, please take a look, I might have missed something |
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.
Pretty much the only thing left is that I think the resources
in Tasks
should have a type as well, but I'm happy to merge this as-is and keep iterating!
/approve
/lgtm
/hold
(Feel free to remove /hold and merge as is!)
- resourceRef: | ||
name: resource-name | ||
name: workspace | ||
- name: workspace |
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 think this should have a type as well, in this case git
or github
, so that a person using the Task knows what kind of resource to provide
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.
will add type, makes sense for clarity
value: github.com/kubernetes/examples | ||
- name: revision | ||
value: HEAD | ||
value: github.com/GoogleCloudPlatform/redis-docker/blob/master/4/debian9/4.0/Dockerfile |
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.
hmm, do you think folks would often have git
resources that point at just one file, or would they be more likely to specify the whole repo?
I'm guessing that even if the goal here was to use this Dockerfile
, the Task
would need other files in the repo as well, so maybe this would be something like value: github.com/GoogleCloudPlatform/redis-docker
and the Taskwould also need a param with the path to the Dockerfile,
/4/debian9/4.0/Dockerfile`?
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.
yes, you are correct, the url should not include the path to the file
- name: guestbookImage | ||
key: builtImage # bind to the name in the task | ||
resourceRef: | ||
name: guestbookstagingimage |
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.
cool, looking good!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish, pivotal-nader-ziada 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 |
@bobcatfish made the final changes /hold cancel |
@bobcatfish you will have to |
/lgtm |
/lgtm |
* cleanup to resource types - rename `StandardResource` to `PipelineResource` - rename `ResourceSuper` to `PipelineResourceInterface` and make func public - change to PipelineResourceSpec to not be an array * update example yaml with better resource names * updates to resources design remove resources from pipeline main yaml add resourceRef to source binding remove unused name from ResourceSpec update examples * add type field to resource in task
Proposed Changes
StandardResource
toPipelineResource
ResourceSuper
toPipelineResourceInterface