Allow setting Dashboard image tag during Capact release

[pkosiec]

Description

Changes proposed in this pull request:

• Allow setting Dashboard image tag during Capact release
• Refactor make-release.sh script to use yq (as previous approach with sed was error prone)

Testing

Sample run: https://github.com/pkosiec/capact/actions/runs/1751316086

(I commented unnecessary steps: pkosiec@71cc872)

Results:

• pkosiec@8471202
• pkosiec@7b434e3

Related issue(s)

#600

[mszostok]

I tested that and overall it works, but the only thing that I don't like about it is that it breaks our formatting: mikefarah/yq#465 and produces less readable vaules.yaml files.

Personally, I'm not sure whether this change is really needed. The current solution worked IMO better. I know that it was error-prone, but this is not a user facing script.

Maybe we should just have our own tool to replace proper things in our manifests.

[pkosiec]

While I understand that yq reformats yamls removing e.g. blank lines, it still keep comments and other values, which doesn't seem to me as a huge downside. So I have to disagree with

The current solution worked IMO better. I know that it was error-prone, but this is not a user facing script.

The current solution is very shortsighted. In case of Dashboard tag, it forces me to replace any tag property, which is a generic word. It's a matter of time when we have such duplicated property in the values.yaml file. Same with other properties, like branch, which could occur multiple times, as we have multiple sources support in Populator. Should we build more complex sed one-liners in future? Definitely not.

Maybe we should just have our own tool to replace proper things in our manifests.

And this sounds like an overkill for me, but of course you can contribute with such solution 🙂 I backed off with the yq refactor (reverted modifications in two files) to end discussion, as I need the new functionality for upcoming release.

So, here are examples after changes:

• Failed run (image with PR- prefix`): https://github.com/pkosiec/capact/runs/4953623108?check_suite_focus=true
• Successful run: https://github.com/pkosiec/capact/runs/4953647640?check_suite_focus=true
• Results:
  • pkosiec@eb9de8f
  • pkosiec@738bbb4

(After that I fixed linter issues, but I also tested the script locally, it works the same).

[mszostok]

I totally get your concerns and I'm not against fixing that at all 👍
But I think that it will be good to just log an issue for it instead of doing that here. We should aim for a solution which will solves the problem that you described but also preserve current good formatting approach 👍

And this sounds like an overkill for me,

I was referring to rewriting bash to Go which IMO is not so overkill at all as we are quite fluent in Go. Having:

# DASHBOARD_IMAGE_TAG - Dashboard image tag used for a given release
[[ ( -z "${DASHBOARD_IMAGE_TAG}" || "${DASHBOARD_IMAGE_TAG}" == PR-* ) ]] && echo "Need to set DASHBOARD_IMAGE_TAG that doesn't start with 'PR-' prefix" && exit 1;

SOURCE_BRANCH="$(git rev-parse --abbrev-ref HEAD)"
RELEASE_VERSION_MAJOR_MINOR="$(echo "${RELEASE_VERSION}" | sed -E 's/([0-9]+\.[0-9])\.[0-9]/\1/g')"
RELEASE_BRANCH="release-${RELEASE_VERSION_MAJOR_MINOR}"

is definitely not our thing.

And this sounds like an overkill for me, but of course you can contribute with such solution 🙂

As mentioned, we can log an issue for that.

Regarding mikefarah/yq#465, it was not working like that previously, so definitely there will be a solution for that.

TL;DR;
I'm totally not against improvements, and you spot a real problem but let's not do them in a rush where we solve one problem but introduce another one. If you can, please create a ticket for it, so we can come up with a solution and introduce it also in all repos at once. As we all known "consistent code is easier to maintain, and requires less cognitive overhead."

BTW previous solution was also approved, but as you revert it I also approve this one 👍