Instructions to handle breaking changes (#452) #702

ynvich · 2024-08-23T16:25:20Z

ynvich commented

2024-08-23 16:25:20 +00:00

First-time contributor

Fix top-level README and provide detailed instructions to handle breaking changes of default config.

ynvich added 1 commit 2024-08-23 16:25:20 +00:00

Instructions to handle breaking changes (#452 )

check-and-test / check-and-test (pull_request) Has been cancelled

Details

4caefecfc9

ynvich requested review from justusbunsi 2024-08-23 16:25:20 +00:00

ynvich requested review from pat-s 2024-08-23 16:25:20 +00:00

pat-s commented

2024-08-25 13:34:28 +00:00

Thanks for contributing.

I don't see why we should outsource the upgrade instructions into a separate document at the moment.
The proposed changes are somewhat specific to your setup and will not apply to others. In addition, you used a lot "in my case" combined with "we" -> while the former should be avoided in general, the latter suggests it comes from the gitea team or maintainers, which is not the case and hence somewhat misleading.

As many different configurations are in place, the current upgrade instructions focus on being somewhat generic. We can't cover all different configuration cases of (older) installations. Users are required to apply the right steps to a certain degree for their own installations.

Setting queue.TYPE to "channel" is also not a desired change and won't work in the proposed state, as queue.CONN_STR is still set to a memcache con string.

Overall, I'd not merge this PR and would argue that these notes are internal/personal upgrade notes and do not fit within the chart README. But also would like to hear @justusbunsi opinion on this.

Thanks for contributing. I don't see why we should outsource the upgrade instructions into a separate document at the moment. The proposed changes are somewhat specific to your setup and will not apply to others. In addition, you used a lot "in my case" combined with "we" -> while the former should be avoided in general, the latter suggests it comes from the gitea team or maintainers, which is not the case and hence somewhat misleading. As many different configurations are in place, the current upgrade instructions focus on being somewhat generic. We can't cover all different configuration cases of (older) installations. Users are required to apply the right steps to a certain degree for their own installations. Setting `queue.TYPE` to "channel" is also not a desired change and won't work in the proposed state, as `queue.CONN_STR` is still set to a memcache con string. Overall, I'd not merge this PR and would argue that these notes are internal/personal upgrade notes and do not fit within the chart README. But also would like to hear @justusbunsi opinion on this.

ynvich commented

2024-08-26 13:53:21 +00:00

First-time contributor

I don't see why we should outsource the upgrade instructions into a separate document at the moment.
The proposed changes are somewhat specific to your setup and will not apply to others.

I faced this while updating a default v6 chart, it shoudn't be that rare.

Setting queue.TYPE to "channel" is also not a desired change and won't work in the proposed state, as queue.CONN_STR is still set to a memcache con string.

The instructions in current README.md are broken as is. queue.TYPE "memcache" just won't work. In addition, cache and session parts required plain host:port, not a memcache URL.

> I don't see why we should outsource the upgrade instructions into a separate document at the moment. > The proposed changes are somewhat specific to your setup and will not apply to others. I faced this while updating a default `v6` chart, it shoudn't be that rare. > Setting `queue.TYPE` to "channel" is also not a desired change and won't work in the proposed state, as `queue.CONN_STR` is still set to a memcache con string. The instructions in current `README.md` are broken as is. `queue.TYPE` "memcache" just won't work. In addition, `cache` and `session` parts required plain `host:port`, not a memcache URL.

pat-s commented

2024-08-29 09:32:28 +00:00

I faced this while updating a default v6 chart, it shoudn't be that rare.

My comment was not about the version you're coming from, but more about the individual settings and actions you've described.

The instructions in current README.md are broken as is. queue.TYPE "memcache" just won't work.

I can't follow your reasoning here. The part you're addressing in your PR has queue.TYPE = "memcache" and queue.CONN_STR = "<memcache connection string>". What would not work here? Of course, this assumes you have deployed memcache standalone, as mentioned in the previous paragraph.

In addition, cache and session parts required plain host:port, not a memcache URL.

This should be updated, I agree, likely with a link to the configuration cheat sheet showing the correct notation.

> I faced this while updating a default v6 chart, it shoudn't be that rare. My comment was not about the version you're coming from, but more about the individual settings and actions you've described. > The instructions in current README.md are broken as is. queue.TYPE "memcache" just won't work. I can't follow your reasoning here. The part you're addressing in your PR has `queue.TYPE = "memcache"` and `queue.CONN_STR = "<memcache connection string>"`. What would not work here? Of course, this assumes you have deployed `memcache` standalone, as mentioned in the previous paragraph. > In addition, cache and session parts required plain host:port, not a memcache URL. This should be updated, I agree, likely with a link to the [configuration cheat sheet](https://docs.gitea.com/administration/config-cheat-sheet#cache-cache) showing the correct notation.

ynvich commented

2024-08-30 10:05:36 +00:00

First-time contributor

My comment was not about the version you're coming from, but more about the individual settings and actions you've described.

I have described a default configuration for a v6 chart, where only gitea.config.server is configured. I still think I am not the only person facing a broken default configuration with no working recipe to handle the upgrade.

The instructions in current README.md are broken as is. queue.TYPE "memcache" just won't work.

I can't follow your reasoning here.

This is broken:

gitea:  config:  queue:  TYPE: memcache  CONN_STR: memcache://gitea-memcached.gitea.svc.cluster.local:11211/

This works:

gitea:  config:  queue:  TYPE: channel  CONN_STR: memcache://gitea-memcached.gitea.svc.cluster.local:11211/

In addition, cache and session parts required plain host:port, not a memcache URL.

This should be updated, I agree, likely with a link to the configuration cheat sheet showing the correct notation.

I have no preference which way it is fixed. Feel free to treat this PR as a bug report. I just followed this request

> My comment was not about the version you're coming from, but more about the individual settings and actions you've described. I have described a default configuration for a `v6` chart, where only `gitea.config.server` is configured. I still think I am not the only person facing a broken default configuration with no working recipe to handle the upgrade. > > The instructions in current README.md are broken as is. queue.TYPE "memcache" just won't work. > > I can't follow your reasoning here. This is broken: ```yaml gitea: config: queue: TYPE: memcache CONN_STR: memcache://gitea-memcached.gitea.svc.cluster.local:11211/ ``` This works: ```yaml gitea: config: queue: TYPE: channel CONN_STR: memcache://gitea-memcached.gitea.svc.cluster.local:11211/ ``` > > In addition, cache and session parts required plain host:port, not a memcache URL. > > This should be updated, I agree, likely with a link to the [configuration cheat sheet](https://docs.gitea.com/administration/config-cheat-sheet#cache-cache) showing the correct notation. I have no preference which way it is fixed. Feel free to treat this PR as a bug report. I just followed this [request](https://gitea.com/gitea/helm-chart/issues/452#issuecomment-740920)

pat-s commented

2024-08-30 10:23:36 +00:00

The latter isn't a valid config. In this case, CONN_STR is just ignored as TYPE: channel doesn't expect one. Which is why it is "working".

I already understood from your previous comments that the documentation hints passing a memcache connection string for queue.CONN_STR, which won't work.
Thanks for reporting, we will update it.

I have described a default configuration for a v6 chart, where only gitea.config.server is configured. I still think I am not the only person facing a broken default configuration with no working recipe to handle the upgrade.

I am not sure that we fully understand each other in this case. Anyhow, my opinion on this still stands: I don't think this part should be merged.

The latter isn't a valid config. In this case, `CONN_STR` is just ignored as `TYPE: channel` doesn't expect one. Which is why it is "working". I already understood from your previous comments that the documentation hints passing a memcache connection string for `queue.CONN_STR`, which won't work. Thanks for reporting, we will update it. > I have described a default configuration for a v6 chart, where only gitea.config.server is configured. I still think I am not the only person facing a broken default configuration with no working recipe to handle the upgrade. I am not sure that we fully understand each other in this case. Anyhow, my opinion on this still stands: I don't think this part should be merged.

ynvich closed this pull request

2024-08-30 11:44:33 +00:00

ynvich commented

2024-08-30 12:18:46 +00:00

First-time contributor

The latter isn't a valid config. In this case, CONN_STR is just ignored as TYPE: channel doesn't expect one. Which is why it is "working".

It seems memcache is not a supported queue type at all.

 modules/queue/workerqueue.go	Line 195 in e5e40787dc 
   func getNewQueueFn(t string) (string, func(cfg *BaseConfig, unique bool) (baseQueue, error)) {

> The latter isn't a valid config. In this case, `CONN_STR` is just ignored as `TYPE: channel` doesn't expect one. Which is why it is "working". It seems `memcache` is not a supported queue type at all. https://gitea.com/gitea/gitea-mirror/src/commit/e5e40787dcce40f69d3489adca2a80cf685a3fe8/modules/queue/workerqueue.go#L195

pat-s commented

2024-08-30 13:00:30 +00:00

Correct, it should likely be redis here.

Correct, it should likely be [redis here](https://docs.gitea.com/administration/config-cheat-sheet#queue-queue-and-queue).

check-and-test / check-and-test (pull_request) Has been cancelled

Required

Details

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gitea/helm-gitea#702

Allow edits from maintainers