Enable admin user password creation/update mode in values #677

2024-07-04T18:40:35Z

solacelost commented

2024-07-04 18:40:35 +00:00

(Migrated from gitea.com)

Description of the change

This enables sane modes for forcing reset, as well as providing more options to users of the chart by giving them the flexibility to set the mode for password creation/modification as part of init whether the user exists or not.

Benefits

The new default should revert to the behavior before #673 became an issue, while also providing more flexibility for users who want to be able to manage their initial admin user password out-of-band after creating it the first time.

Possible drawbacks

None that I can think of.

Applicable issues

fixes #673

Additional information

See the discussion in #675 as well

Checklist

Parameters are documented in the values.yaml and added to the README.md using readme-generator-for-helm

### Description of the change This enables sane modes for forcing reset, as well as providing more options to users of the chart by giving them the flexibility to set the mode for password creation/modification as part of init whether the user exists or not. ### Benefits The new default should revert to the behavior before #673 became an issue, while also providing more flexibility for users who want to be able to manage their initial admin user password out-of-band after creating it the first time. ### Possible drawbacks None that I can think of. ### Applicable issues - fixes #673 ### Additional information See the discussion in #675 as well ### Checklist - [X] Parameters are documented in the `values.yaml` and added to the `README.md` using [readme-generator-for-helm](https://github.com/bitnami-labs/readme-generator-for-helm)

❤️ 1

pat-s (Migrated from gitea.com) reviewed 2024-07-05 09:53:10 +00:00

pat-s (Migrated from gitea.com) left a comment

Thanks for the effort!

I like the initial idea to make the option configurable.
However, I'd prefer using helpers.tpl to define passwordMode and I am not sure if we really need a schema.json to solve this.

Further, I think we should document these options in more detail in the README as well, as it is a central configuration setting, e.g. under https://gitea.com/gitea/helm-chart#admin-user.

It might also be that we need to check for the gitea version in use, i.e. < 1.22 does not have the flag and might error subsequently. That means if users have a custom gitea version set, it won't work with that new chart version. This in turn means that this change will also result in a major chart release.

I'd also first like to get @justusbunsi reply on this.

Sorry that this might delay things a bit but for the time being you can already install the chart from this branch or stick with Gitea 1.21 for the time being.
It might also be worthwhile to make the init-container image configurable, this would also allow to workaround this particular issue more easily.

Thanks for the effort! I like the initial idea to make the option configurable. However, I'd prefer using `helpers.tpl` to define `passwordMode` and I am not sure if we really need a `schema.json` to solve this. Further, I think we should document these options in more detail in the README as well, as it is a central configuration setting, e.g. under https://gitea.com/gitea/helm-chart#admin-user. It might also be that we need to check for the gitea version in use, i.e. < 1.22 does not have the flag and might error subsequently. That means if users have a custom gitea version set, it won't work with that new chart version. This in turn means that this change will also result in a major chart release. I'd also first like to get @justusbunsi reply on this. Sorry that this might delay things a bit but for the time being you can already install the chart from this branch or stick with Gitea 1.21 for the time being. It might also be worthwhile to make the `init-container` image configurable, this would also allow to workaround this particular issue more easily.

solacelost commented

2024-07-05 13:53:09 +00:00

(Migrated from gitea.com)

I've addressed two of your three points:

I shifted from a schema-based enum to validation with fail in _helpers.tpl
I beefed up the explicit documentation of the feature around the admin password docs

With regards to the third, I'd like to know what your thoughts are on the best way to implement this. If accepting that we'll only support 1.22 would be preferred to get a chart out fast, vs longer-term flexibility, then I'll document the breaking change.

If there's some other approach that is decided on, then I'm happy to do it :)

I'm not positive what the best method for determining the Gitea version would be as I, for example, use the -rootless image. I'm not sure if coupling the chart logic to tag suffix parsing and handling would be a happy path, so I'm hopeful there's another way.

I've addressed two of your three points: - I shifted from a schema-based enum to validation with `fail` in `_helpers.tpl` - I beefed up the explicit documentation of the feature around the admin password docs With regards to the third, I'd like to know what your thoughts are on the best way to implement this. If accepting that we'll only support 1.22 would be preferred to get a chart out fast, vs longer-term flexibility, then I'll document the breaking change. If there's some other approach that is decided on, then I'm happy to do it :) I'm not positive what the best method for determining the Gitea version would be as I, for example, use the `-rootless` image. I'm not sure if coupling the chart logic to tag suffix parsing and handling would be a happy path, so I'm hopeful there's another way.

👍 1

justusbunsi commented

2024-07-05 15:41:24 +00:00

(Migrated from gitea.com)

It might also be that we need to check for the gitea version in use, i.e. < 1.22 does not have the flag and might error subsequently. That means if users have a custom gitea version set, it won't work with that new chart version. This in turn means that this change will also result in a major chart release.

@pat-s Fair point. That is indeed an issue. Applying these changes to 1.21.11 causes the init container to crash-loop with Incorrect Usage: flag provided but not defined: -must-change-password on Pod recreation. First deploy works.

With regards to the third, I'd like to know what your thoughts are on the best way to implement this. [...] I'm not positive what the best method for determining the Gitea version would be as I, for example, use the -rootless image. I'm not sure if coupling the chart logic to tag suffix parsing and handling would be a happy path, so I'm hopeful there's another way.

@solacelost The init container has access to gitea --version. So we can on-the-fly detect the version. I've crafted a snippet that will detect 1.22+. Feel free to integrate it into your PR 🙂. I think with such check we can mitigate the breaking change.

// See https://gitea.com/gitea/helm-chart/issues/673
// "gitea --version" produces an output of the format:
// "Gitea version <major>.<minor>.<patch> built with ...."
local cli_breaking_minor_version=22
local version_regex="(Gitea version\s+)\d+\.(\d+)\.\d+(.*)"
if [[ "$(gitea --version)" =~ $version_regex ]]; then
  if [[ ${BASH_REMATCH[2]} -ge $cli_breaking_minor_version ]]; then
    echo 'handle version with new flag'
  else
    echo 'handle older versions'
  fi
fi

> It might also be that we need to check for the gitea version in use, i.e. < 1.22 does not have the flag and might error subsequently. That means if users have a custom gitea version set, it won't work with that new chart version. This in turn means that this change will also result in a major chart release. @pat-s Fair point. That is indeed an issue. Applying these changes to 1.21.11 causes the init container to crash-loop with `Incorrect Usage: flag provided but not defined: -must-change-password` on Pod recreation. First deploy works. > With regards to the third, I'd like to know what your thoughts are on the best way to implement this. [...] I'm not positive what the best method for determining the Gitea version would be as I, for example, use the -rootless image. I'm not sure if coupling the chart logic to tag suffix parsing and handling would be a happy path, so I'm hopeful there's another way. @solacelost The init container has access to `gitea --version`. So we can on-the-fly detect the version. I've crafted a snippet that will detect 1.22+. Feel free to integrate it into your PR 🙂. I think with such check we can mitigate the breaking change. ```bash // See https://gitea.com/gitea/helm-chart/issues/673 // "gitea --version" produces an output of the format: // "Gitea version <major>.<minor>.<patch> built with ...." local cli_breaking_minor_version=22 local version_regex="(Gitea version\s+)\d+\.(\d+)\.\d+(.*)" if [[ "$(gitea --version)" =~ $version_regex ]]; then if [[ ${BASH_REMATCH[2]} -ge $cli_breaking_minor_version ]]; then echo 'handle version with new flag' else echo 'handle older versions' fi fi ```

solacelost commented

2024-07-05 16:14:17 +00:00

(Migrated from gitea.com)

@justusbunsi Would you be opposed to altering that to be a little more flexible and instead of a hard pivot on the version number checking the gitea admin user change-password --help with a plain-text match for --must-change-password? I don't know why I didn't think of that before, but thinking about how fragile a version match is at all made it come to mind.

@justusbunsi Would you be opposed to altering that to be a little more flexible and instead of a hard pivot on the version number checking the `gitea admin user change-password --help` with a plain-text match for `--must-change-password`? I don't know why I didn't think of that before, but thinking about how fragile a version match is at all made it come to mind.

❤️ 1

justusbunsi commented

2024-07-05 16:34:10 +00:00

(Migrated from gitea.com)

@solacelost Fair point about the version match fragility. I haven't thought about grepping for the flag. So, I have absolutely nothing against your idea.

@solacelost Fair point about the version match fragility. I haven't thought about grepping for the flag. So, **I have absolutely nothing against your idea**.

👍 1

solacelost commented

2024-07-05 16:43:15 +00:00

(Migrated from gitea.com)

Done and looks to behave on this end - I'll keep watching to see if anything else comes up. Thanks for good communication fixing this :)

👍 1

justusbunsi (Migrated from gitea.com) approved these changes 2024-07-05 17:44:34 +00:00

justusbunsi (Migrated from gitea.com) left a comment

@william-elastisys Thanks for reporting the bug and your initial PR. @solacelost Thanks for your initiative to immediately implement the configuration idea. It's a feature that fixes a bug. Love it 😆. We should communicate this in the release notes. @pat-s Anything from your side before merging?

justusbunsi commented

2024-07-07 09:58:47 +00:00

(Migrated from gitea.com)

Going to merge and release now.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lunny/helm-chart#677