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.

Sign up for GitHub

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

Jump to bottom

Add enum like values for all type placeholders #18

Closed
bobcatfish opened this issue Sep 7, 2018 · 0 comments · Fixed by #54
Closed

Add enum like values for all type placeholders #18

bobcatfish opened this issue Sep 7, 2018 · 0 comments · Fixed by #54
Assignees
@bobcatfish

Comments

@bobcatfish
Copy link
Collaborator

bobcatfish commented Sep 7, 2018 •
edited
Loading

Expected Behavior

Whenever a type attribute is used, there should be string aliases for all the possible value values so we can see what values should be possible. This should just include the types we would initially expect to implement.

Actual Behavior

There are a bunch of places in our API where we use a type attribute, but it's not clear what kinds of values should actually be used for these, for example https://github.com/knative/build-pipeline/blob/ef113e1d513c67b692eb1096904789be0e9616b6/pkg/apis/pipeline/v1beta1/pipelineparams_types.go#L35

Additional Info

According to k8s API conventions we should not use enums, but should use string constants instead, e.g. see knative/serving ServiceConditionType
@bobcatfish bobcatfish self-assigned this Sep 7, 2018
bobcatfish referenced this issue in bobcatfish/pipeline Sep 17, 2018
@bobcatfish
Add values for types in resources
16912c0 
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
@bobcatfish bobcatfish mentioned this issue Sep 17, 2018
Merged
knative-prow-robot pushed a commit that referenced this issue Sep 18, 2018
@bobcatfish @knative-prow-robot
Add values for types in resources (#54)
6c65e99 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bobcatfish bobcatfish

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add values for types in resources bobcatfish/pipeline
1 participant
@bobcatfish