# Shipwright Beta API Workshop

## Full build strategy with all possible fields

**NOTE**: DO NOT EDIT before workshop unless something is missing.

---
apiVersion: shipwright.io/v1alpha1
kind: ClusterBuildStrategy
metadata:
  name: strategy-name
spec:
  parameters:
    - name: array-argument
      description: "The description of the parameter."
      # optional, default string
      type: array
      # optional
      defaults: []
    - name: string-argument
      description: "The description of the parameter."
      type: string
      # optional
      default: registry
  volumes:
    - name: some-volume
      description: "The description of the volume."
      # optional, default false
      overridable: true
      # is just an example, the whole VolumeSource is possible here
      emptyDir: {}
  steps:
    # These are the _only_ options supported, this is not the full Container spec
    - name: some-name
      image: alpine:latest
      imagePullPolicy: Always
      workingDir: $(params.shp-source-root)
      command:
        - /some-command
      args:
        - some-arg
      env:
        - name: ENV_NAME
          value: SOME_VALUE
      securityContext:
        runAsUser: 0
        capabilities:
          add:
            - CHOWN
            - DAC_OVERRIDE
      resources:
        requests:
          cpu: 100m
          memory: 128Mi
        limits:
          cpu: 1m
          memory: 2Gi
      volumeMounts:
        - name: some-volume
          mountPath: /somepath          
          readOnly: false

## Full build with all possible fields

**NOTE**: DO NOT EDIT before workshop unless something is missing.

apiVersion: shipwright.io/v1alpha1
kind: Build
metadata:
  name: some-build
  annotations:
    # optional, default "false"
    build.shipwright.io/verify.repository: "true"
spec:
  trigger:
    when:
      - name: a-github-trigger
        type: GitHub
        github:
          events:
            - PullRequest|Push
          # optional
          branches:
            - branch-1
            - branch-2
      - name: an-image-trigger
        type: Image
        image:
          names:
            - image-1
            - image-2
      - name: a-pipeline-trigger-based-on-name
        type: Pipeline
        objectRef:
          name: some-pipeline-name
          # optional
          status:
            - SomeStatus
      - name: a-pipeline-trigger-based-on-labels
        type: Pipeline
        objectRef:
          selector:
            aLabel: aValue
          # optional
          status:
            - SomeStatus
    # optional
    triggerSecret: webhook-token-secret
  # Bundle
  source:
    ociArtifact:
      image: ghcr.io/shipwright-io/sample-go/source-bundle:latest
      # optional, default Never
      prune: AfterPull|Never
      pullSecret: my-registry-secret
    contextDir: docker-build
  # Git
  source:
    git:
      url: [e-mail]:shipwright-io/build.git
      # optional, default is default branch
      revision: v0.10.0
      # optional
      cloneSecret: my-ssh-secret
    # optional
    contextDir: docker-build
  strategy:
    name: buildah
    # optional, default ClusterBuildStrategy
    kind: BuildStrategy
  # optional
  paramValues:
    - name: simple-string
      value: "Some value"
    - name: string-from-configmap
      configMapValue:
        name: configmap-name
        key: key-in-configmap
        # optional
        format: KEY=${CONFIGMAP_VALUE}
    - name: string-from-secret
      secretValue:
        name: secret-name
        key: key-in-secret
        # optional
        format: KEY=${SECRET_VALUE}
    - name: array-with-everything
      value:
        - value: simple-item
        - configMapValue:
            name: configmap-name
            key: key-in-configmap
            # optional
            format: KEY=${CONFIGMAP_VALUE}
        - secretValue:
            name: secret-name
            key: key-in-secret
            # optional
            format: KEY=${SECRET_VALUE}
  # optional
  volumes:
    - name: my-volume-override
      # is just an example, the whole VolumeSource is possible here
      emptyDir: {}
  # optional
  env:
    # We here use the Kubernetes EnvVar
    - name: AN_ENV
      value: "My value"
    - name: POD_NAME
      valueFrom:
        # More options available in EnvVarSource
        fieldRef:
          fieldPath: ".metadata.name"
  # optional, default 10m
  timeout: 5m
  output:
    image: image-registry.openshift-image-registry.svc:5000/build-examples
    /taxi-app
    # optional
    pushSecret: some-registry-secret
    # optional
    annotations:
      some.domain/my-annotation: "Some annotation value"
    # optional
    labels:
      myLabel: "Custom label value"
  # optional
  retention:
    # optional - "false" means that a BuildRun is deleted when the parent Build is deleted
    keepOrphaned: true
    # optional
    failedLimit: 3
    # optional
    succeededLimit: 3
    # optional
    ttlAfterFailed: 48h
    # optional
    ttlAfterSucceeded: 8h

## Full buildrun with all possible fields

**NOTE**: DO NOT EDIT before workshop unless something is missing.

apiVersion: shipwright.io/v1alpha1
kind: BuildRun
metadata:
  name: some-buildrun
spec:
  
  build:
    # optional, mutually exclusive with spec
    name: some-build
    # optional, mutually exclusive with name
    spec: {} # see Build.spec
  # optional, mutually exclusive with build.spec
  paramValues: [] # see Build.paramValues
  # optional, mutually exclusive with build.spec
  env: [] # see Build.env
  # optional, default with name=pipeline and fallback to default
  # .generated is a special value, replaces `generate: true`
  serviceAccount: ".generated"
  # optional, mutually exclusive with build.spec
  timeout: 5m
  # optional
  state: BuildRunCanceled
  # optional, mutually exclusive with build.spec
  retention:
    # optional
    ttlAfterFailed: 48h
    # optional
    ttlAfterSucceeded: 8h
  # optional, mutually exclusive with build.spec
  volumes: [] # see Build.volumes
status:
  buildSpec: {} # see Build.spec
  taskRunName: some-buildrun-taskrun
  startTime: 2022-02-22T22:22:22Z
  completionTime: 2022-02-22T22:22:22Z
  conditions:
    - type: Succeeded
      status: True|False|Unknown
      lastTransitionTime: 2022-02-22T22:22:22Z
      reason: Succeeded|BuildNotFound|...
      message: "Some explanatory text."
  # deprecated
  failedAt:
    pod: some-buildrun-taskrun-pod
    container: step-build-and-push
  failureDetails:
    location:
      pod: some-buildrun-taskrun-pod
      container: step-build-and-push
    reason: BuildStrategyDefinedFailureReason
    message: "Explanatory details for the reason."
  source:
    git:
      commitSha: abcd
      commitAuthor: "Some Name"
      branchName: main
  output:
    digest: sha:....
    size: 2196391

## API conventions

* Optional non-array fields in spec are always pointers.
* Use unique `name` fields in arrays to the furthest extent possible. Utilize `patchMergeKey=name` and `patchStrategy=merge,retainKeys` annotations to enable `kubectl patch` on arrays.
*

## List of proposed change requests:

### Example: Title

Scope: API Convention|BuildStrategy|Build|BuildRun
Originator: Put in your name
What: Put in your proposal. Note: this is about changing our API objects to become nicer and consistent. It is NOT about introducing features.
Migration: assess if the old version can be migrated to the new version and vice versa without any loss of information

### Add convention about lists different elements - ACCEPTED

Scope: API Convention
Originator: Sascha
What: Today, we are inconsistent in that. Basically it is about all arrays that we have which contain definitions of different kinds. I suggest that we:

* Always have a `name` field. This is also helpful for `kubectl patch` I think to correlate items if we also specify the following on the array property:

	// +patchMergeKey=name
	// +patchStrategy=merge,retainKeys

~~* do NOT have a kind or type field. Inherited pattern for volumes is the example; type is implicitly defined.~~ Moved to separate proposal.
* Have different keys for the different element kinds, all are pointers, exactly one must be set.

Examples:

* We follow this pattern today already for `spec.volumes`.

  spec:
    volumes:
      - name: some-volume
        hostPath:
          path: /data
      - name: some-other-volume
        emptyDir: {}

* We should imo do this for `spec.sources`, see [below](#Build-sources)
* We should imo do this for `spec.triggers` = remove `spec.trigger.when[].type`

Migration: no problem in both directions because type is redundant information.

### Add convention regarding 3rd party service credentials - ACCEPTED

Scope: API Convention
Originator: Adam Kaplan
What: We currently use a `credentials` object to reference a secret that used for authentication and authorization with 3rd party services. This proposes replacing the object reference with a simple field. Each field indicates the reason why the secret is provided.

git clone example:

spec:
  source:
    git:
      url: <>
      cloneSecret: my-git-creds

image push example:

spec:
  output:
    image: image-name
    pushSecret: my-push-creds

Concern - using `<reason>Secret` will result in false positives from code scanners.
`<reason>SecretName` may result in fewer false positives. This was tested and not confirmed.

### Rename buildSteps to steps - ACCEPTED

Scope: BuildStrategy
Originator: Sascha
What: I think that `steps` is shorter but still descriptive enough. For consistency we can then rename the underlying Go type from `BuildStep` to `Step`
Migration: no problem in both directions

### Remove inline Container in BuildStep - ACCEPTED

Scope: BuildStrategy
Originator: Sascha
What: change `BuildStep` to not anymore have an inline `Container` object, but instead put in only those fields that we copy into the TaskRun. Those today are `name`, `image`, `imagePullPolicy`, `workingDir`, `command`, `args`, `env`, `resources`, `securityContext`, and `volumeMounts`. It will then look like this:

spec:
  buildSteps:
    - name: some-name
      image: alpine:latest
      # Optional
      imagePullPolicy: Always
      # Optional
      workingDir: $(params.shp-source-root)
      # Optional
      command:
        - /some-command
      # Optional
      args:
        - some-arg
      # Optional
      env:
        - name: ENV_NAME
          value: SOME_VALUE
      # Optional
      resources:
        limits:
          cpu: "1"
          memory: 1G
      # Optional
      securityContext:
        runAsUser: 0
        capabilities:
          add:
            - CHOWN
            - DAC_OVERRIDE
      # Optional
      volumeMounts:
        - name: some-volume
          mountPath: /somepath          
          readOnly: false

Migration: Possible in both directions. When migrating from alpha to beta we potentially throw away other fields that could theoretically be defined such as `EnvFrom` or `LivenessProbe`, but given we never forwarded those to the TaskRun anyway, we do not loose anything. In the future, we can revisit if we should forward additional fields that would make sense such as for example `Lifecycle`.

Notes: We should try and keep our API independent of dependencies (k8s, Tekton), unless we are comfortable fully supporting a given dependent field.
Examples - not exposing the `script` field in Tekton steps, limiting `EnvFrom`.

### Move build-run-deletion annotation to spec.retention - ACCEPTED

Scope: Build
Originator: Sascha
What: introduce `.spec.retention.atBuildDeletion` of type `bool` with default `false`
Migration: no problem in both directions

kind: Build
spec:
  retention:
    keepOrphaned: true
    failedLimit: 5
    succeededLimit: 5

Suggestion - change default behavior to set the owner reference on BuildRuns, use `retention` field to opt out of this behavior.

Interim is `spec.retention.keepOrphaned`. Community can continue to brainstorm names for this field.

### Build sources - DECLINED in favor of "Revert sources"

Scope: Build
Originator: Sascha
What: Remove source, go forward with sources, and move contextDir to `.spec.strategy` (or `.spec`?):

spec:
  # optional (means no sources is valid)
  sources:
    - name: a-git-source
      git:
        url: [e-mail]:shipwright-io/build.git
        # optional, default is default branch
        revision: v0.10.0
        # optional
        # TBD: how ?
        secretName: my-ssh-secret
    - name: an-http-source
      http:
        url: https://raw.githubusercontent.com/shipwright-io/community/main/assets/icons/png/shipwright-logo-1color-darkbg-128.png
    - name: a-bundle-source
      container:
        url: ghcr.io/shipwright-io/sample-go/source-bundle:latest
        # optional
        # TBD: how ?
        secretName: my-ssh-secret
    - name: a-local-source
      local:
        # optional
        timeout: 30s
  strategy:
    # optional
    contextDir: my-context-dir

Migration: there will only be problems if somebody defines a Build with a Git and a Bundle source, or with more than one of them. This cannot be handled. But we will need to discuss whether this is at all allowed anyway.

### Revert sources, only have `spec.source` - ACCEPTED

Scope: Build
Originator: Adam Kaplan
What: A counter-proposal to "Build Sources" - we should only support a single source per build/buildRun, but allow multiple types within `source`.

git example:

spec:
  source:
    # optional, if the working directory should be a subdirectory within the given source
    contextDir: app
    type: Git
    git:
      url: [e-mail]:shipwright-io/build.git
      # optional, default is default branch
      revision: v0.10.0
      # optional, needed for private repositories
      cloneSecret: my-ssh-secret

local example:

spec:
  source:
    type: Local
    contextDir: src
    local:
      timeout: 5m

bundle (container image) example. Rename to "ociArtifact":

spec:
  source:
    type: ContainerImage
    # optional, if the input source should be a subdirectory within the container image
    contextDir: /app-root/bundle
    containerImage:
      image: ghcr.io/shipwright-io/sample-go/source-bundle:latest
      # optional, needed for private registries
      pullSecret: my-ghcr-pull-secret
      prune: true # existing API
      

Migration: Git and bundleContainer source builds will be able to migrate, perhaps with some re-mapping of YAML keys.
Local source builds will switch to using `spec.source.local` instead of `spec.sources[0].localCopy`.
HTTP sources would be dropped as a feature and no longer supported.
Builds with `spec.sources` and `spec.source` will not be able to migrate. Question - how common is this?

Suggestions:
- Move `contextDir` to be top level (under "source")
- BuildRun status should have `status.source`, where relevant information is populated. Example: `status.source.git.sha`, `status.source.ociArtifact.digest`

### Eliminate dockerfile field - ACCEPTED

Scope: Build
Originator: Sascha
What: Remove `.spec.dockerfile`
Migration: we will add a dockerfile parameter to all sample build strategies for Dockerfile-based builds. We can also continue to set the unofficial `$(params.DOCKERFILE)` for some time until we remove it to not break old build strategies. The rest is doable.

Note - some concerns regarding parameter and migration.

### Eliminate builder field - ACCEPTED

Scope: Build
Originator: Sascha
What: Remove `.spec.builder`
Migration: we will modify the s2i sample build strategies and add a parameter for the image name a volume for the secret. But I do not see how we can support a good migration path here especially in the build strategy.

### Eliminate volume description on Build - ACCEPTED

Scope: Build
Originator: Sascha
What: remove `.spec.volumes[].description`. This brings it in synchronization with parameters where the description is only provided in the strategy and cannot be overwritten.
Migration: not possible because of removal

### Remove ~~`registered`~~ status from Build and BuildStrategy/ClusterBuildStrategy - ACCEPTED

Scope: Build
Originator: Adam Kaplan
What: Remove `status` from objects which do not result in executing code - `Build`, `BuildStrategy`, and `ClusterBuildStrategy`.
Migration: No direct API migration. Clients which report this information should drop these fields.

Notes:
- Current checks verify that the Build object is internally consistent.
- We have a feature where a Secret can be watched to re-validate Build objects.
- Validations should happen when BuildRun is created.

TODO: Deprecate `status` in alpha API?

### Rename latestTaskRunRef to taskRunName - ACCEPTED

Scope: BuildRun
Originator: Sascha
What: rename `.status.latestTaskRunRef` to `.status.taskRunName`
Migration: no problem in both directions

Notes:
- Potential new API convention - should references to objects include `Name` in the suffix? Otherwise `.status.taskRun` would be viable.
Results of the detect-secrets investigation will inform this decision.

### Remove generated service account from API - ACCEPTED

Scope: BuildRun
Originator: Adam Kaplan
What: Remove API-level support for generating a service account. Instead, use a string reference to a service account in a BuildRun, and eventually the Build object as well. Allow the ".generated" service account value, which continues the existing behavior until it is deprecated/removed.
Migration: serviceAccount.generated = true can move to `serviceAccount = .generated`.

spec:
  serviceAccount(Name): my-service-account
  serviceAccount(Name): .generated # compat value until we implemented the generated service account removal

Notes:
- Eventually allow to define it also on the Build
- Motivation of this feature is that it allows dedicated service accounts to set up image push secrets via Tekton's creds-init feature. We don't want to pollute service accounts with linked secrets.
- We need to keep the existing behavior, but this can be done with a special ".generated" value for the service account name. Once we remove the dependency on creds-init, we can drop this feature by deprecating/removing support for the ".generated" service account (name).
- ".generated" is not a valid Kubernetes object reference, so we can use it as a special value.

### Consolidate buildRef and buildSpec - ACCEPTED

Scope: BuildRun
Originator: Sascha
What: move the referenced (`.spec.buildRef.name`) or inline build specification (`.spec.buildSpec`) into one top-level. One can then specify either

spec:
  build:
    name: a-build

or

spec:
  build:
    spec: {} # full build spec

This makes it behave consistent with `spec.strategy` on the Build.

Migration: no problem in both directions

### Drop Kind/Type fields - DECLINED

Scope: API Convention
Originator: Sascha
What: do NOT have a kind or type field. Inherited pattern for volumes is the example; type is implicitly defined.

### Use Kind/Type fields as type discriminators - ACCEPTED

Scope: API Convention
Originator: Adam Kaplan
What: Use "type discriminators" if an API object has some form of implied polymorphism. For example, source types or volume sources.

Counter-proposal to "Drop Kind/Type fields"

Problem: volumeSource is not maintained by us so we would have a problem to provide a type field - we could introduce our own volumeSource with selected options only.

Proposal: only introduce type field when we own the options (for source and triggers but not for volume sources) -> we go that path