fix: add annotations to ensure new docker image is downloaded

[zaremba-tomasz]

To ensure new Docker image being downloaded after a push to a branch we need to make changes to the knative service manifest. The following annotations are added on each deployment (initium.nearform.com/updateSha and initium.nearform.com/updateTimestamp for spec.template.metadata.annotations) to make sure repository changes are reflected in the cluster.

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  annotations:
    serving.knative.dev/creator: system:serviceaccount:default:initium-cli-sa
    serving.knative.dev/lastModifier: system:serviceaccount:default:initium-cli-sa
  creationTimestamp: "2023-10-02T10:51:29Z"
  generation: 3
  name: initium-nodejs-demo-app
  namespace: initium-chore-another-pr
  resourceVersion: "8393"
  uid: cf8ef898-4092-4f3f-b0cf-2dca6b979e65
spec:
  template:
    metadata:
      annotations:
        initium.nearform.com/updateSha: 3eb1ba79fea98373648953266a9c502f1c88849c
        initium.nearform.com/updateTimestamp: "2023-10-02T12:56:34+02:00"
      creationTimestamp: null
    spec:
      containerConcurrency: 0
      containers:
      - image: ghcr.io/zaremba-tomasz/initium-nodejs-demo-app:initium-chore-another-pr
        imagePullPolicy: Always
        name: user-container
        readinessProbe:
          successThreshold: 1
          tcpSocket:
            port: 0
        resources: {}
      timeoutSeconds: 300
  traffic:
  - latestRevision: true
    percent: 100

Closes #103

[LucaLanziani]

Looking at this again I think this can be simplified.

The GetManifestDigest func uses RemoteTag internally and looking at the code I believe there is no need to persist the digest in the DockerImage at all.

So imho you could just rename GetManifestDigest to GetImageDigest and return the whole RemoteTags()[digest] and use it directly.

Of course doing this will make the test part harder but I would be ok not to have a test for now and write an integration test that only run on CI next.

[zaremba-tomasz]

@LucaLanziani I'm not really sure about this.

In my understanding digest should be an integral part of the RemoteTag() as knative requires that. I assume there will be some day when DockerImage will be decoupled from DockerService and every time the struct will be initialized then digest will be fetched as well. In such case I (as a user of this interface) would be surprised that the value returned from the RemoteTag() is not something ready to be used.

WDYT?

[LucaLanziani]

@zaremba-tomasz digest is not part of RemoteTag imho, RemoteTag before this change if I'm not mistaken was used in two places, Push and service.yaml.tmpl, the main reason to need a RemoteTag was to push the container to a registry e only after was used by the service manifest to deploy since it looked like a good fit.

The Push will never need the digest and now we are saying that the Deploy will always need the digest.

This to me is a clear indication of separation of concern, the RemoteTag should not even know what a digest is imho.

[zaremba-tomasz]

@LucaLanziani please take a look whether this is something you were thinking about. If so I'll test this locally and extend knative test as described during standup.

[LucaLanziani]

You might want to check if the user is set here, since this is needed when pulling from a private registry

[LucaLanziani]

I also just realised that this will fail if we pull from a private registry since we dont have any registry-user registry-password parameter in the deploy command.

https://github.com/nearform/initium-cli/pull/117/files#diff-53781244989afadf4bf128fdfd32e6bb60771477c410da74e6ace9dc1c5e8ae9R35

[LucaLanziani]

This is getting to complicated, are you sure that a new revision will not pull the new image?

[LucaLanziani]

looking at kn service update behaviour it looks like they always create a new revision, should probably do the same here or even use the kn client library itself if we find an easy way to integrate it.

[LucaLanziani]

it looks like they are using an annotation to do this knative/client#1364
I'm sorry @zaremba-tomasz but I'm thinking of dropping this and use the annotation approach :(

Here how to setup the annotation, we should probably use the same annotation to be compatible and eventually try to use their library for this.

[LucaLanziani]

Actually @zaremba-tomasz I was thinking that having a SHA of the commit that generated the change would be way more useful, the issue is when you run the CLI from your local machine and maybe you have uncommitted changes, at the point the value could be lastCommitSha-dirty? https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitglossary.html#def_dirty

[zaremba-tomasz]

This is getting to complicated, are you sure that a new revision will not pull the new image?

The initial issue was that, the subsequent push to the repository did not create new revision at all (most probably because manifest did not change).

[zaremba-tomasz]

Actually @zaremba-tomasz I was thinking that having a SHA of the commit that generated the change would be way more useful, the issue is when you run the CLI from your local machine and maybe you have uncommitted changes, at the point the value could be lastCommitSha-dirty? https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitglossary.html#def_dirty

The issue was not related to the local environment only; I can easily reproduce this in GHA as well.

[LucaLanziani]

The initial issue was that, the subsequent push to the repository did not create new revision at all (most probably because manifest did not change).

Yes, it looks like that changing the annotation value will also trigger a new revision that will pull the image again,