♻️ Deduplicate definitions from deployment and cronjob #62

ddelange · 2022-07-05T08:24:11Z

Based on #28

Closes #63

Added one commit to bring the branch up to date with latest upstream

ref ddelange#1 for testing purposes, this can be installed with:

helm install --namespace ... https://github.com/ddelange/docker-registry.helm/releases/latest/download/helm-chart-docker-registry-2.2.0.tgz

ddelange · 2022-07-05T08:24:47Z

cc @canterberry @kuzaxak

ddelange · 2022-08-03T20:40:46Z

@canterberry what do you think? its opt-in, non-breaking, should be safe to merge?

canterberry · 2022-08-03T22:16:53Z

First: thank you! I know you don't have to contribute anything at all, but you did! I and the community are grateful, and I don't show that gratitude early and often enough. So thank you.

Now, let's talk about how we can get this done!

Your general approach seems fine to me -- a new configuration block, clearly documented in the README, and disabled by default. When enabled, there's a cronjob that runs at some configurable frequency, using the registry's own CLI and configuration to perform the garbage collection. Great!

Can you distill that down into a [near-]completely additive changeset?

Rationale: You've done some refactoring here that goes above and beyond, which is great holistically, but in terms of code review, makes it much more difficult to evaluate the impact the changeset will have on existing code. For this chart in particular, I'm very hesitant to review any changes that aren't a slam-dunk, because I'm super cautious about breaking it, and since I'm not a user, personally, I rely on tidy changesets and easy code reviews to build that confidence to ship.

Another small thing: no need to increment the version in Chart.yaml. That's something I do as part of the release process, in case there are multiple PRs ready for merge around the same time.

Let's go! 🎉

ddelange · 2022-08-04T06:36:16Z

Hi @canterberry 👋

For this chart in particular, I'm very hesitant to review any changes that aren't a slam-dunk, because I'm super cautious about breaking it, and since I'm not a user, personally, I rely on tidy changesets and easy code reviews to build that confidence to ship.

I can totally understand.

Can you distill that down into a [near-]completely additive changeset?

If you do a ctrl+F for include in the diff of this PR, there will be hits in both deployment.yaml and cronjob.yaml which require the same blocks. Turning this PR into an additive changeset would basically mean copy-pasting all of deployment.yaml into cronjob.yaml, and any future changes would need to happen in both places.

If you do a ctrl+F for define in the diff of this PR, you'll see the three blocks that were moved from deployment.yaml to _helpers.tpl (1:1 without changes) so that they can be used by both yamls via include.

I don't want to try to convince you to merge a PR you don't feel safe merging, but maybe there's a way of getting you a diff view that helps you review the PR. If you'd open these side by side, and do a visual inspect, maybe the triviality of the change becomes evident and we can get closer to that slam-dunk level? :)

envs
- before
- after
volumeMounts
- before
- after
volumes
- before
- after

I'll revert the version bump in any case. Please let me know what you think!

ddelange · 2022-08-04T06:50:45Z

Would CI help boost confidence? e.g. https://github.com/helm/chart-testing-action

I think their example workflow would show that there are no changes to the rendered deployment.yaml resulting from this PR

canterberry · 2022-08-04T23:40:09Z

Thank for your diligence. I can tell you're trying to make it easier for me, and I appreciate the effort! It's not what I need, though, so I'd like to redirect that energy toward achieving our mutual goal of getting this welcome feature delivered.

In order to get this PR merged, please make it [near-]completely additive. No refactoring, no formatting changes. Just what is strictly necessary to deliver the feature as described. Only then will I feel comfortable reviewing the changeset. At that point, you might expect some minor feedback or changes requested, but that cycle time would be much shorter and I think we could get this merged quite quickly.

Just so you know, I have done some due diligence here and am not being blindly dismissive! I've tried reviewing the changeset multiple times, even again today, and it's exceedingly difficult to do with any degree of confidence because the changeset is not as identical as you describe. There are many minor deviations -- collectively a non-trivial refactoring. Further, without a comprehensive test suite to evaluate the correctness of all classes of variance in the chart output, an equality check on a single known output is insufficient to provide a satisfactory degree of confidence (although any such test case would be an improvement over nothing at all, and I'd love to have such a suite in this highly configurable chart!)

Edit: I completely understand your argument on minimizing duplication, and I agree with you! I'd love to see that in a follow-up PR once this one is merged, if you're willing! Tightly scoped refactoring, on its own, is pretty easy to review when it's not coupled with functional changes.

This reverts commit fd7fdca.

ddelange · 2022-08-05T12:18:42Z

Hi @canterberry,

I've opened #68 with additional commit fd7fdca.

Once that merges, this PR will become a refactor-PR, with as diff the revert commit d78c010.

Is that what you are aiming for?

canterberry · 2022-08-09T17:51:38Z

@ddelange Once you rebase and/or merge the latest into this PR, I'll take a thorough look. I think it makes sense, conceptually, that the CronJob's pod(s) and the Deployment's pod(s) run with matching environments and volumes. The only potential issue I see coming up in review is whether (and to what extent) any functional changes or regressions might have slipped in. None have yet caught my eye, though.

templates/deployment.yaml

README.md

canterberry · 2022-08-09T20:22:09Z

To inspect the changes in this PR, I ran the following command from a local clone of this PR's branch:

$ helm template --namespace meta-namespace --set namespace=target-namespace  --set garbageCollect.enabled=true drc .

In both the resulting CronJob and Deployment manifests, there were a couple of extra newlines preceding the env block:

              env: 
                
                
                - name: REGISTRY_HTTP_SECRET
                  valueFrom:
                    secretKeyRef:
                      name: drc-docker-registry-secret
                      key: haSharedSecret
                - name: REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY
                  value: "/var/lib/registry"

While this doesn't seem to have any functional impact (helm upgrade --install --dry-run ... with the same arguments comes back successful), it's something that looks visually off, and I'd like to see that fixed. Other than that, I've finished a review pass and LGTM. I'd like to let it "bake" for a day or two to give others a change to review as well, just in case we've missed something.

ddelange · 2022-08-09T20:34:26Z

templates/_helpers.tpl

+{{- end }}
+
+{{- with .Values.extraEnvVars }}
+        {{- toYaml . | nindent 12 }}


need to test, think indentation of extraEnvVars will come out wrong

Suggested change

{{- toYaml . | nindent 12 }}

{{- toYaml . }}

confirmed with

helm template --debug . --set extraEnvVars[0].name=TEST_NAME --set extraEnvVars[0].value=TEST_VALUE

ddelange

I think this should do the trick ref

templates/cronjob.yaml

templates/deployment.yaml

kuzaxak

Small notes from me. Generally LGTM. Thank you for picking up my PR :)

templates/_helpers.tpl

kuzaxak · 2022-08-10T06:27:03Z

templates/_helpers.tpl

+  value: {{ .Values.s3.secure | quote }}
+{{- end }}
+
+{{- else if eq .Values.storage "swift" }}


If you have time you can split this chunk of variables into separate define sections, one per type, and then render them inside that define.

In that case you will achieve function like style where every Backend storage have isolated function to render their env variables.

Just a note, existing solution is fine as well.

templates/_helpers.tpl

ddelange · 2022-08-10T07:09:07Z

env whitespaces fixed in 52a5d4b

tested with

helm template --debug . \
  --set serviceAccount.create=true \
  --set priorityClassName=high \
  --set podAnnotations.test=annotation \
  --set extraEnvVars[0].name=TEST_NAME \
  --set extraEnvVars[0].value=TEST_VALUE \
  --set secrets.htpasswd=abc \
  --set tlsSecretName=abc \
  --set garbageCollect.enabled=true \
  --set proxy.enabled=true \
  --set storage=s3 /
  --set secrets.s3.secretKey=abc /
  --set secrets.s3.accessKey=def /
  --set s3.region=us-42 /
  --set s3.bucket=abc /
  --set s3.encrypt=abc

ddelange · 2022-08-10T08:52:47Z

I think we're not there yet. This might be a sensible Github Workflow to comment (and edit that comment) with a diff on a PR:

alias helm_test_template='helm template --debug \
  --set serviceAccount.create=true \
  --set priorityClassName=high \
  --set podAnnotations.test=annotation \
  --set extraEnvVars[0].name=TEST_NAME \
  --set extraEnvVars[0].value=TEST_VALUE \
  --set secrets.htpasswd=abc \
  --set tlsSecretName=abc \
  --set garbageCollect.enabled=true \
  --set proxy.enabled=true \
  --set storage=s3 /
  --set secrets.s3.secretKey=abc /
  --set secrets.s3.accessKey=def /
  --set s3.region=us-42 /
  --set s3.bucket=abc /
  --set s3.encrypt=abc'
diff -u <(helm_test_template https://github.com/twuni/docker-registry.helm/archive/refs/heads/main.tar.gz) <(helm_test_template .)

gives in the ```diff syntax:

--- /dev/fd/63	2022-08-10 10:48:29.000000000 +0200
+++ /dev/fd/62	2022-08-10 10:48:29.000000000 +0200
@@ -12,7 +12,7 @@
     release: RELEASE-NAME
 type: Opaque
 data:
-  haSharedSecret: "S2pTbHFaeEtCZ2ppTHY3bQ=="
+  haSharedSecret: "TnZBakJNSWVpbXRKeWFSUw=="
   proxyUsername: ""
   proxyPassword: ""
 ---
@@ -99,7 +99,7 @@
         release: RELEASE-NAME
       annotations:
         checksum/config: 581c8cf6d59449d6c89ebf8ec1bef05751a9c8e281ca31f99c4e0291e054bfaf
-        checksum/secret: 153f24ee9a430e533197ba266c83c185b4f95d38cd6ee547c93eca76a1969b85
+        checksum/secret: 25156342abc38ff3b671e698c348aa9d5726d62a1a9bcb539437febc7e5b8ccc
     spec:
       securityContext:
         fsGroup: 1000
@@ -122,9 +122,9 @@
             httpGet:
               path: /
               port: 5000
-          resources:
+          resources:
             {}
-          env:
+          env:
             - name: REGISTRY_HTTP_SECRET
               valueFrom:
                 secretKeyRef:
@@ -132,14 +132,14 @@
                   key: haSharedSecret
             - name: REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY
               value: "/var/lib/registry"
-          volumeMounts:
-            - name: data
-              mountPath: /var/lib/registry/
+          volumeMounts:
             - name: "RELEASE-NAME-docker-registry-config"
               mountPath: "/etc/docker/registry"
-      volumes:
-        - name: data
-          emptyDir: {}
+            - name: data
+              mountPath: /var/lib/registry/
+      volumes:
         - name: RELEASE-NAME-docker-registry-config
           configMap:
             name: RELEASE-NAME-docker-registry-config
+        - name: data
+          emptyDir: {}

ddelange · 2022-08-10T09:14:29Z

^ opened #70

canterberry

👍 LGTM. Whitespace looks good now, with my limited-scope smoke test.

canterberry · 2022-08-15T21:53:21Z

Released in v2.2.1.

ddelange mentioned this pull request Jul 5, 2022

Solve merge conflicts for twuni#28 x-qdo/docker-registry.helm#1

Closed

1003n40 previously approved these changes Jul 20, 2022

View reviewed changes

ddelange dismissed 1003n40’s stale review via 3b4ff67 August 4, 2022 06:45

ddelange mentioned this pull request Aug 5, 2022

Add option to enable garbage collector #68

Merged

Revert "Make the changes additive"

d78c010

This reverts commit fd7fdca.

PR Suggestions

fb2132a

Merge branch 'main' into garbage-collector

f22f42d

ddelange changed the title ~~Add option to enable garbage collection~~ ♻️ Deduplicate definitions from deployment and cronjob Aug 9, 2022

canterberry reviewed Aug 9, 2022

View reviewed changes

templates/deployment.yaml Show resolved Hide resolved

ddelange commented Aug 9, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

🔥 Remove merge remnant

5a831ab

ddelange commented Aug 9, 2022

View reviewed changes

kuzaxak reviewed Aug 10, 2022

View reviewed changes

Fix extraEnvVars indentation, remove env whitespaces

52a5d4b

Fix more indentations

b4b3194

ddelange mentioned this pull request Aug 10, 2022

👷 Add sticky helm diff PR comment #70

Merged

canterberry mentioned this pull request Aug 15, 2022

Chart always expects accessKey and secretKey to be defined when using s3 storage #71

Open

canterberry approved these changes Aug 15, 2022

View reviewed changes

canterberry merged commit 471cb14 into twuni:main Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Deduplicate definitions from deployment and cronjob #62

♻️ Deduplicate definitions from deployment and cronjob #62

ddelange commented Jul 5, 2022 •

edited

Loading

ddelange commented Jul 5, 2022

ddelange commented Aug 3, 2022

canterberry commented Aug 3, 2022

ddelange commented Aug 4, 2022

ddelange commented Aug 4, 2022 •

edited

Loading

canterberry commented Aug 4, 2022 •

edited

Loading

ddelange commented Aug 5, 2022

canterberry commented Aug 9, 2022

canterberry commented Aug 9, 2022

ddelange Aug 9, 2022

ddelange Aug 10, 2022 •

edited

Loading

ddelange left a comment •

edited

Loading

kuzaxak left a comment

kuzaxak Aug 10, 2022

ddelange commented Aug 10, 2022 •

edited

Loading

ddelange commented Aug 10, 2022

ddelange commented Aug 10, 2022

canterberry left a comment

canterberry commented Aug 15, 2022

♻️ Deduplicate definitions from deployment and cronjob #62

♻️ Deduplicate definitions from deployment and cronjob #62

Conversation

ddelange commented Jul 5, 2022 • edited Loading

ddelange commented Jul 5, 2022

ddelange commented Aug 3, 2022

canterberry commented Aug 3, 2022

ddelange commented Aug 4, 2022

ddelange commented Aug 4, 2022 • edited Loading

canterberry commented Aug 4, 2022 • edited Loading

ddelange commented Aug 5, 2022

canterberry commented Aug 9, 2022

canterberry commented Aug 9, 2022

ddelange Aug 9, 2022

Choose a reason for hiding this comment

ddelange Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

ddelange left a comment • edited Loading

Choose a reason for hiding this comment

kuzaxak left a comment

Choose a reason for hiding this comment

kuzaxak Aug 10, 2022

Choose a reason for hiding this comment

ddelange commented Aug 10, 2022 • edited Loading

ddelange commented Aug 10, 2022

ddelange commented Aug 10, 2022

canterberry left a comment

Choose a reason for hiding this comment

canterberry commented Aug 15, 2022

ddelange commented Jul 5, 2022 •

edited

Loading

ddelange commented Aug 4, 2022 •

edited

Loading

canterberry commented Aug 4, 2022 •

edited

Loading

ddelange Aug 10, 2022 •

edited

Loading

ddelange left a comment •

edited

Loading

ddelange commented Aug 10, 2022 •

edited

Loading