Skip to content
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

Add values for types in resources #54

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

bobcatfish
Copy link
Collaborator

Added values for the following types, which would be the initial types
we would want to implement and support (could expand to support more in
the future):

  • SourceType
  • ArtifactType
  • ResultTargetType
  • PipelineTriggerType and TaskTriggerType
  • ParamType (just strings for now?)

Since in #39 @pivotal-nader-ziada and @shashwathi are combining Sources
and Artifacts into one concept, we'll likely combine SourceType and
ArtifactType into one type as well. (Sorry @pivotal-nader-ziada and @shashwathi
this will probably cause some conflicts with your changes but hopefully they'll
be minor to resolve! Lemme know if I can help or if you want me to hold off)

While doing this noticed and cleaned up a couple things:

  • Don't need to specify lists of result stores, just one
    for each type (test, log, runs). Also these are identical
    except for their usage, so we can use a common type for all of them
  • Turns out pipeline run specs were not defined!
  • Updated DEVELOPMENT.md re. how to regenerate generated code
    (such as zz_generated.deepcopy.go)

Fixes #18

Added values for the following types, which would be the initial types
we would want to implement and support (could expand to support more in
the future):
- `SourceType`
- `ArtifactType`
- `ResultTargetType`
- `PipelineTriggerType` and `TaskTriggerType`
- `ParamType` (just strings for now?)

Since in #39 @pivotal-nader-ziada and @shashwathi are combining Sources
and Artifacts into one concept, we'll likely combine `SourceType` and
`ArtifactType` into one type as well.

While doing this noticed and cleaned up a couple things:
- Don't need to specify lists of result stores, just one
  for each type (test, log, runs). Also these are identical
  except for their usage, so we can use a common type for all of them
- Turns out pipeline run specs were not defined!
- Updated DEVELOPMENT.md re. how to regenerate generated code
  (such as `zz_generated.deepcopy.go`)

Fixes #18
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 17, 2018
@nader-ziada
Copy link
Member

we will rebase this once its merged and can resolve any conflicts, there are a lot of good clean up here

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018
Copy link
Member

@nader-ziada nader-ziada left a comment

Choose a reason for hiding this comment

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

Maybe we can have a generic interface for result store to make extensible, with runs, logs and tests as implementations of this interface?

Copy link
Contributor

@shashwathi shashwathi left a comment

Choose a reason for hiding this comment

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

I like the TaskTriggerRef idea a lot. This is critical information for an operator to know the cause of task run. Great addition.

My Canadian 2 cents: Is ResultTarget a good candidate for interface. Defining the expected behavior rather than restricting it to uploading to a destination. One use case I can think of would be an operator desire to dump logs in DB.

/lgtm
Rest looks great. Awesome work 👍

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM

@knative-prow-robot knative-prow-robot merged commit 6c65e99 into tektoncd:master Sep 18, 2018
@bobcatfish
Copy link
Collaborator Author

Is ResultTarget a good candidate for interface. Defining the expected behavior rather than restricting it to uploading to a destination. One use case I can think of would be an operator desire to dump logs in DB.

For sure @shashwathi I think that makes a lot of sense and is definitely relevant to @tejal29 's interests as well :D

My Canadian 2 cents

But we don't even have pennies anymore 😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add enum like values for all type placeholders
5 participants