search

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
113 stars 24 forks source link

kustomize: replace patchesStrategicMerge with patches #311

Closed ofan closed 1 year ago

ofan commented 1 year ago

kustomize v5.0.0 deprecated patchesStrategicMerge, it's renamed to patches. Normally it just gives a warning, but some tools like Argocd will treat this as error. https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/patchesstrategicmerge/

anoopcs9 commented 1 year ago

/test centos-ci/sink-clustered/mini-k8s-latest

ofan commented 1 year ago

Thanks @anoopcs9 . I'm still new to this. Just added commit message with sign-off. make check-gitlint seems good (no errors or output).

anoopcs9 commented 1 year ago

@ofan You'll have to split the entire line into commit subject, an empty line and body with line length limited to 80 characters as below:

kustomize: replace patchesStrategicMerge with patches

kustomize v5.0.0 deprecated patchesStrategicMerge, it's renamed to
patches. Normally it just gives a warning, but some tools like Argocd
will treat this as error.

https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/patchesstrategicmerge/

Signed-off-by: ofan <319679+ofan@users.noreply.github.com>

Even then the check is expected to fail as the url itself is over the 80 characters limit. But I think we can ignore it for now. Can you update the commit message along the above suggestion to get rid of the first two from the following list of errors?

2: B1 Line exceeds max length (162>80): "kustomize v5.0.0 deprecated patchesStrategicMerge, it's renamed to patches. Normally it just gives a warning, but some tools like Argocd will treat this as error."

2: B4 Second line is not empty: "kustomize v5.0.0 deprecated patchesStrategicMerge, it's renamed to patches. Normally it just gives a warning, but some tools like Argocd will treat this as error."

3: B1 Line exceeds max length (92>80): "https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/patchesstrategicmerge/"

obnoxxx commented 1 year ago

Thanks @anoopcs9 . I'm still new to this. Just added commit message with sign-off. make check-gitlint seems good (no errors or output).

Thanks for the update, @ofan !

The gitlint check still fails, so I am afraid that you need to polish the commit message once more, as @anoopcs9 explained in his comment. Additionally, I am not entirely sure that the email address you used in the signoff line is appropriate or even valid: it seems to be the automatic github email. do you have a more "official" email address you could use for your git authorship and signoff? Note that this is nothing that gitlin complains about, it just caught my eye ...

ofan commented 1 year ago

Thank you for your patience. I just updated commit based on suggestions. gitlint didn't complain except for the url

$ gitlint
6: B1 Line exceeds max length (92>80): "https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/patchesstrategicmerge/"
phlogistonjohn commented 1 year ago

Thank you for your patience. I just updated commit based on suggestions. gitlint didn't complain except for the url

$ gitlint
6: B1 Line exceeds max length (92>80): "https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/patchesstrategicmerge/"

This is good enough for me. We can merge without making gitlint happy. Let's adjust the gitlint rules and document how to put in long urls in a later PR. If the change looks good to you (@obnoxxx, @anoopcs9, et al) please approve regardless of what gitlint thinks.

obnoxxx commented 1 year ago

approving after the merge.

Thanks for your effort and contribution to improve our codebase, @ofan !

LGTM

The github UI goes not seem to support approving of a merged PR .... 🤷🏼