Revert "nixos/gitea: set service type to notify"

[RaitoBezarius]

Description of changes

This reverts commit b61919e. of #243883.

As it breaks Forgejo who does not support this feature yet.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[emilylange]

@ofborg test gitea forgejo

[Ma27]

I gotta say, I'm not a very big fan of this. Can't we create a forgejo module which does these kinds of fixups and uses services.gitea internally? Or at least do an if-else on cfg.package for that?

Disclaimer: my personal opinion of forgejo isn't very high. To me it mostly seems like a rebranded clone that releases a few days to weeks after gitea and I'm not very faithful that such a fork will last in the long-term, so I'm not very keen on having to having to look after it whenever I have to touch the gitea module & package.

But to be clear my stance is heavily influenced by my personal - granted, rather negative - opinion (and I wouldn't call it objective because of that), so if everyone else has a different take on this, I'm fine with getting overruled here.

[emilylange]

ofborg test failure on x86_64-linux is unrelated.

[emilylange]

Quoting @RaitoBezarius from #244840

We made the decision in the past to share Gitea and Forgejo codebase [...]

I guess we could reconsider that.
I am not against separating them.
If anything, probably slightly in favor of doing so.

But it would lower the Bus factor slightly.

cc @urandom2 @bendlas

[RaitoBezarius]

If you will, let's do a separation and we can revert the revert, once everything is good, does that work for you all?

[SuperSandro2000]

Can we please not hastily revert things because they broke something somewhere else when we alternatives like making things conditional? Thanks

[SuperSandro2000]

With this easy change we can easily support both gitea and forgejo

[SuperSandro2000]

Suggested change

	Type = "simple";
	Type = if (lib.versionAtLeast (lib.versions.majorMinor cfg.package.version) "1.20") then "notify" else "simple";

nix-repl> if (lib.versionAtLeast (lib.versions.majorMinor gitea.version) "1.20") then "notify" else "simple"
"notify"

nix-repl> if (lib.versionAtLeast (lib.versions.majorMinor forgejo.version) "1.20") then "notify" else "simple"
"simple"

[RaitoBezarius]

Have you read the discussion?

[RaitoBezarius]

Can we please not hastily revert things because they broke something somewhere else when we alternatives like making things conditional? Thanks

No, reverts are fine whenever they break something because they were merged hastily.

[RaitoBezarius]

@ofborg test gitea forgejo

[RaitoBezarius]

@Ma27 is it okay for you if we proceed with this revert and then we follow up with the split, then revert the revert?

[Ma27]

No, reverts are fine whenever they break something because they were merged hastily.

With all due respect, I have to disagree here:

• the new version was running on at least my infra for a few days
• there was no indication from CI that something's off
• I'm pretty sure that I wasn't even really aware of the situation: just checked, I'm not subscribed to the init PR (forgejo: init at 1.18.0-rc1-1 #207796) and I've seen an alternative PR earlier which also added a new module (I think I've seen that the tests were recycled somehow, but I haven't payed close attention here, because I'm not a forgejo maintainer)

@Ma27 is it okay for you if we proceed with this revert and then we follow up with the split, then revert the revert?

Go for it.

Me being unhappy with the situation is not a reason to let forgejo admins suffer (considering that we have a workaround that is OK and also quite trivial) 😅

However, for the future we should do some things differently:

• it should be made sure that on CI runs both forgejo and gitea get tested as long as parts of the module are shared
• I'd like to see a "forgejo-fixup.nix" (or something similar) that basically makes sure that forgejo works fine (e.g. systemd.services.gitea.serviceConfig.Type = mkIf (cfg.package.pname == "forgejo") (mkForce "simple") - just as an example) that should be maintained by the forgejo maintainers.

Also, I don't really have the time (nor am I really interested in investing time into forks of the software I use), so I'm not up for the job I'm afraid.

[RaitoBezarius]

No, reverts are fine whenever they break something because they were merged hastily.

With all due respect, I have to disagree here:

• the new version was running on at least my infra for a few days
• there was no indication from CI that something's off
• I'm pretty sure that I wasn't even really aware of the situation: just checked, I'm not subscribed to the init PR (forgejo: init at 1.18.0-rc1-1 #207796) and I've seen an alternative PR earlier which also added a new module (I think I've seen that the tests were recycled somehow, but I haven't payed close attention here, because I'm not a forgejo maintainer)

Ma27, I think you do a great job and I believe you very well tested the Gitea part, I think what is important to see here is how Forgejo maintainers feels regarding the whole situation.

A PR was opened to bump the Gitea version, this is usually fine as long as it does not touch the Gitea module as it was decided in #210790 (review) by @SuperSandro2000 to not share the module. Moving on, you introduced 2 days ago the change that caused this breakage here: #243883 (comment) and it was merged yesterday after an approval 3 hours after.

This is a premature merge from the views of Forgejo maintainers and a frustrating one because they are not put in reviewers list, they have to actively monitor any change and they don't necessarily proactively set code owners to get pinged.

Now, I can agree there is a communication problem lurking, but from end-users perspective, those changes could have led to more problematic issues. This is why I qualify this as a premature merge and one that is usually reverted as soon as possible because it also reduce the breakage until all stakeholders can discuss and figure out a path forward.

The whole problem with "something in CI is off" is hard to use as an argument when ofborg is broken and I believe you when you say you were not aware of the situation.

Again, no offense — you are doing, in my opinion and experience, an amazing job, nevertheless, I sympathize with @emilylange here who can be understandably unhappy with the whole situation and this is not your "fault" per-se.
So please do not take this personally.

@Ma27 is it okay for you if we proceed with this revert and then we follow up with the split, then revert the revert?

Go for it.

Me being unhappy with the situation is not a reason to let forgejo admins suffer (considering that we have a workaround that is OK and also quite trivial) sweat_smile

However, for the future we should do some things differently:

• it should be made sure that on CI runs both forgejo and gitea get tested as long as parts of the module are shared
• I'd like to see a "forgejo-fixup.nix" (or something similar) that basically makes sure that forgejo works fine (e.g. systemd.services.gitea.serviceConfig.Type = mkIf (cfg.package.pname == "forgejo") (mkForce "simple") - just as an example) that should be maintained by the forgejo maintainers.

Also, I don't really have the time (nor am I really interested in investing time into forks of the software I use), so I'm not up for the job I'm afraid.

Let's do a complete split up as it should have happened in that previous PR I mentioned so that both maintainers will not have to interact directly in the same environment.

To all stakeholders: feel free to put load on me to coordinate the split-up/reviews/etc. I want to ensure that everything goes smoothly for everyone and so we can move on.

[SuperSandro2000]

Why did you merge this!?

[SuperSandro2000]

This is a premature merge from the views of Forgejo maintainers and a frustrating one because they are not put in reviewers list, they have to actively monitor any change and they don't necessarily proactively set code owners to get pinged.

Nothing was merged premature. The PR was open for 4 days and contained a security update. We had at least 4 people involved in the reviewing process.

Splitting the module will just duplicate code, as forgejo is just a soft fork which currently shares 99% of the options with gitea.

[bendlas]

How does everybody feel about updating to a forgejo release candidate early? See #244866

[Ma27]

For completeness I'd also like to leave a message here: even though it got a little more heated than usual in here this thread was good to understand each others' issues in the entire gitea/forgejo topic.

My concern was that people would get a wrong impression of how things were handled on the initial package bump[1], but it was also an important reminder for me that things became pretty frustrating for the forgejo maintainers recently (and that's totally understandable!) which is why I attempted to reduce the tensions in #244866 .

Yes, that's just a summary of things that I felt were good to be mentioned in here as well, but I still felt it's important for full context and because I don't take all of this lightly.

[1] yes, the forgejo thing happened and another regression - see #241497 - was because of a misunderstanding from my side missed and I'm perfectly fine with admitting that! What I stated above and I'd like to restate here is that I think we've taken our job serious in the package bump PR even though things slipped through.