CI-3567: Design Release CRD

[goelozev]

The Release CR is responsible for storing the following configuration:

• targetName - a unique name which identifies a release in a namespace. We intentionally don't use .metadata.name as we intend to generate it out of targetName and revisions
• revisions - a list of revisions (git, image, chart), that form that release

A Release will have the form of:

apiVersion: deploy.gocardless.com/v1alpha1
kind: Release
metadata:
  name: connectivity-3a93868
config:
  targetName: connectivity
  revisions:
    anu:
      id: 7ec506e066721c0e1d6430d8ec1d6519964fa59b
      source: github.com/gocardless/anu.git
      metadata:
        author: platform-engineering
        branch: master
        message: "hello"
      type: git
    gcr:
      id: 5a638c1d1a69bf47f63eed430fa1ea26bc53e7c4
      source: gcr.io/gocardless/connectivity
      type: container_image
      metadata:
        tags:
          - v1.2.5

The Release CRD intentionally doesn't have a .spec path, as there is no desired state for a release; release resources are intended to be created after a release has occurred.

[IsaacJames]

Looks good! Just one small question

[0x0013]

Even though this is already merged, I'm adding a couple of comments for considerations.

I think the history list could be made simpler. Arguably, the most important contents of a history entry, would be:

• SupersededBy
• SupersededFrom (see my comment on this. Name could be better - maybe Previous or PreviousRelease.)

both fields would include the time of transition.

Up for discussion in History:

• Do we need the health status? I think we would only be interested in the last (or current) health status.
• The phase can be implied easily - the release itself either is active or isn't, as indicated by a Condition. Any past historical activations are, by definition, inactive. This also means that we may not even need to store the current active release state, and only store it when the Release becomes inactive (superseded). In this case, we might want to store SupersededFrom in the main status of the Release.
• ID of historical entry - arguable, see my comment. We might want to use it to refer to a specific entry in one of the other SupersededBy fields. But if not, then I'm curious of the purpose of the ID.

Of course, all of these proposals are still up for discussion.

[0x0013]

As mentioned in our discussion and Rollback PR, "phase" status field is considered deprecated, and should use conditions instead:

Some resources in the v1 API contain fields called phase, and associated message, reason, and other status fields. The pattern of using phase is deprecated. Newer API types should use conditions instead.

I do think we should refactor this to use Condition "Active" instead.

[goelozev]

I agree, the Phase should simply become a condition[].type = "Active" with two possible ConditionStatus-es of True and False.

[0x0013]

Same as previous comment - I think it makes sense to use Condition "Healthy" to mark this.

I don't think we need to store the past health state in the history. In general, we will be interested in the latest health state for this Release, in order to determine whether it is a valid rollback candidate. However, there could be an argument made that historical health status might be useful, in case a deployment becomes unhealthy on subsequent evaluation, after already being healthy. However, this would most likely occur in the case of external impact, so I'm not sure if we want to cover that use case.

[goelozev]

Again, agree that this can be and should be a Condition by itself.

However, there could be an argument made that historical health status might be useful, in case a deployment becomes unhealthy on subsequent evaluation, after already being healthy.

I think this could be useful if we run multiple health evaluations, which we aren't planning to do as of now. I would say that it wouldn't hurt to record the previous health statuses, just in case we decide we will need to reason on them for manual and automated rollbacks.

[0x0013]

I wonder if it's worth conforming to the reference field guidelines (some good details in there), with something like:

supersededBy:
  releaseRef: foo
  time: <time>
  id: <id> # optional

I've added id field in the ref, if we want to target a specific ID from the history list - I was wondering what we would use that ID field for, or if it's needed, and this is the only use case that comes to mind. However, it should be sufficient to find it by time, as those would presumably be updated in both resources with the same value.

I'm also wondering whether we might want to also add something like supersededFrom, in order to make it easier to walk back from the currently active release, to the previous ones.

[goelozev]

supersededBy:
  releaseRef: foo
  time: <time>
  id: <id> # optional

Yes, this definitely makes it a bit cleaner. On your question about the id field - as of now, I don't see much value in it, as you already mentioned, the time field would be sufficient to identify the history entry, but having a simple id to identify them in the history might be a better UX.

On your second point about supersededFrom - isn't that the release itself? Or I might have misunderstood!

[goelozev]

@0x0013 I've opened #422 to add conditions. Let's continue the conversation over there!