Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure webhook module #22256

Merged
merged 25 commits into from
Jan 1, 2023

Conversation

delvh
Copy link
Member

@delvh delvh commented Dec 28, 2022

Previously, there was an import services/webhooks inside modules/notification/webhook.
This import was removed (after fighting against many import cycles).
Additionally, modules/notification/webhook was moved to modules/webhook,
and a few structs/constants were extracted from models/webhooks to modules/webhook.

Previously, there was an "import services/webhooks" inside
"modules/notification/webhook".
This import was removed (after fighting against many import cycles).
Additionally, a few constants were extracted from "models/webhooks" to
"modules/notification/webhook".
@delvh delvh added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 28, 2022
@delvh delvh added this to the 1.19.0 milestone Dec 28, 2022
modules/convert/convert.go Outdated Show resolved Hide resolved
modules/notification/notification.go Show resolved Hide resolved
services/webhook/webhook.go Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 28, 2022
@lunny
Copy link
Member

lunny commented Dec 28, 2022

I think modules/webhook is a better place than modules/notification/webhook.

@yardenshoham yardenshoham added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Dec 28, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 28, 2022

What problem does this PR solve? The notification system depends heavily on other parts of our codebase and should therefore be moved into the services packages. It's not a "module" in our definition.

@delvh
Copy link
Member Author

delvh commented Dec 28, 2022

Yes. That's also what I did with the former modules/notification/webhook/webhook.go.
That's now services/webhook/notifier.go.


As to what problem this PR solves
(or at least improves, it was too much too fix all flaws at once):
https://docs.gitea.io/en-us/guidelines-backend/#package-dependencies:

Since Golang don’t support import cycles, we have to decide the package dependencies carefully. There are some levels between those packages. Below is the ideal package dependencies direction.
cmd -> routers -> services -> models -> modules

So, overall, I simply untangled our way too complicated import structure slightly.
The end goal is of course to follow the guideline completely, but as you can see, this single one-line change was already so problematic that the PR to fix it changes 500 lines.


By the way, I can spoiler you that I opened this PR due to a comment I made on my unreleased PR review for #21937.
It also shows you why we need this change: Otherwise even new features like the actions can't be created easily anymore, because you always have to work around the import cycles, which can cost a lot of developers nerves.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 28, 2022
@lunny
Copy link
Member

lunny commented Dec 28, 2022

Yes. That's also what I did with the former modules/notification/webhook/webhook.go. That's now services/webhook/notifier.go.

As to what problem this PR solves (or at least improves, it was too much too fix all flaws at once): https://docs.gitea.io/en-us/guidelines-backend/#package-dependencies:

Since Golang don’t support import cycles, we have to decide the package dependencies carefully. There are some levels between those packages. Below is the ideal package dependencies direction.
cmd -> routers -> services -> models -> modules

So, overall, I simply untangled our way too complicated import structure slightly. The end goal is of course to follow the guideline completely, but as you can see, this single one-line change was already so problematic that the PR to fix it changes 500 lines.

By the way, I can spoiler you that I opened this PR due to a comment I made on my unreleased PR review for #21937. It also shows you why we need this change: Otherwise even new features like the actions can't be created easily anymore, because you always have to work around the import cycles, which can cost a lot of developers nerves.

I think this is a right direction. notification should be kept in modules because it will be used by other services and other services could RegisterNotification so that all code have been put in the right directories. Good work!

modules/webhook/error.go Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 28, 2022
@delvh
Copy link
Member Author

delvh commented Dec 29, 2022

Hmm, why does the CI keep failing consistently at the same location?
I didn't change any behavior, right?
Especially: Why did it work prior to the merge, and since then it complains about a failing test?
The tests are also started using main.go, right?

@zeripath
Copy link
Contributor

As an aside I think notification belongs in services not modules. It clearly depends on models and therefore should be in services.

@delvh delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 30, 2022
@delvh delvh changed the title Remove import services from webhook module Restructure webhook module Dec 30, 2022
services/convert/convert.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #22256 (db7db12) into main (7cc7db7) will decrease coverage by 0.56%.
The diff coverage is 45.34%.

@@            Coverage Diff             @@
##             main   #22256      +/-   ##
==========================================
- Coverage   48.15%   47.58%   -0.57%     
==========================================
  Files        1043     1044       +1     
  Lines      142324   142432     +108     
==========================================
- Hits        68536    67777     -759     
- Misses      65608    66551     +943     
+ Partials     8180     8104      -76     
Impacted Files Coverage Δ
cmd/dump_repo.go 0.00% <ø> (ø)
models/asymkey/gpg_key.go 55.88% <0.00%> (-0.67%) ⬇️
models/asymkey/ssh_key_fingerprint.go 47.36% <0.00%> (ø)
models/asymkey/ssh_key_parse.go 59.44% <0.00%> (ø)
models/asymkey/ssh_key_principals.go 43.66% <0.00%> (ø)
models/db/context.go 63.47% <0.00%> (-1.71%) ⬇️
models/org_team.go 40.55% <0.00%> (ø)
models/packages/conan/references.go 79.31% <ø> (ø)
models/packages/container/search.go 70.40% <ø> (ø)
models/packages/package.go 50.33% <ø> (ø)
... and 196 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lunny
Copy link
Member

lunny commented Jan 1, 2023

make L-G-T-M work

@lunny lunny merged commit 0f4e1b9 into go-gitea:main Jan 1, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 1, 2023
@delvh delvh deleted the maintenance/move-webhook-type-to-modules branch January 1, 2023 15:33
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 3, 2023
* upstream/main:
  Add deprecated warning for DISABLE_GRAVATAR and ENABLE_FEDERATED_AVATAR (go-gitea#22318)
  Unify hashing for avatar (go-gitea#22289)
  fix: code search title translation (go-gitea#22285)
  Update Gmail mailer configuration (go-gitea#22291)
  Fix due date rendering the wrong date in issue (go-gitea#22302)
  Fix get system setting bug when enabled redis cache (go-gitea#22295)
  Restructure `webhook` module (go-gitea#22256)
  Reminder for no more logs to console (go-gitea#22282)
  Fix bug of DisableGravatar default value (go-gitea#22296)
  Upgrade go-chi to v5.0.8 (go-gitea#22304)
  [skip ci] Updated licenses and gitignores
  Use ErrInvalidArgument in packages (go-gitea#22268)
  Changelog v1.18.0 (go-gitea#22215) (go-gitea#22269)
  Support estimated count with multiple schemas (go-gitea#22276)
  Add Gentoo to the from package providers (go-gitea#22284)
  Fix sitemap (go-gitea#22272)
  Add `sync_on_commit` option for push mirrors api (go-gitea#22271)
  Fix key signature error page (go-gitea#22229)
@delvh delvh mentioned this pull request Jan 3, 2023
lunny pushed a commit that referenced this pull request Jan 6, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants