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

Add invlid value check to DISABLED_REPO_UNITS and fix incorrect slice combine #23024

Closed

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Feb 21, 2023

If user set incorrect vaule to DISABLED_REPO_UNITS, all invaild items in DisabledRepoUnits will be Unknown Type 0, and there's no process to check the invaild value in this function.

image

After:
image

@yp05327
Copy link
Contributor Author

yp05327 commented Feb 21, 2023

If there are 2 incorrect values, and the last one in the list is a incorrect value
e.g.

DISABLED_REPO_UNITS = [repo.projects, repo.packages]

we will get an error:
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 21, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Feb 21, 2023

???
image

@yp05327 yp05327 changed the title Add invlid value check to DISABLED_REPO_UNITS Add invlid value check to DISABLED_REPO_UNITS and fix incorrect slice combine Feb 21, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Feb 21, 2023

I don't know whether it is the best way to fix slice combine error in d038707.
But I find there are some same codes:
image

some of them have same logic.
image
-> They are correct: #23024 (comment)

@yp05327 yp05327 force-pushed the add-invalid-vaule-check-to-unit-type branch from 4785964 to d038707 Compare February 21, 2023 02:30
@codecov-commenter
Copy link

Codecov Report

Merging #23024 (d038707) into main (2b3f12f) will increase coverage by 0.00%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main   #23024   +/-   ##
=======================================
  Coverage   47.43%   47.43%           
=======================================
  Files        1139     1140    +1     
  Lines      150707   150790   +83     
=======================================
+ Hits        71491    71532   +41     
- Misses      70755    70788   +33     
- Partials     8461     8470    +9     
Impacted Files Coverage Δ
models/unit/unit.go 50.38% <0.00%> (-2.00%) ⬇️
modules/util/io.go 73.68% <0.00%> (-21.06%) ⬇️
modules/util/timer.go 85.71% <0.00%> (-14.29%) ⬇️
models/auth/token_scope.go 75.00% <0.00%> (-3.95%) ⬇️
modules/charset/charset.go 71.73% <0.00%> (-3.63%) ⬇️
services/pull/comment.go 62.96% <0.00%> (-2.78%) ⬇️
routers/web/repo/setting_protected_branch.go 33.79% <0.00%> (-1.55%) ⬇️
routers/api/v1/user/app.go 66.26% <0.00%> (-0.71%) ⬇️
modules/setting/server.go 50.90% <0.00%> (-0.47%) ⬇️
services/webhook/notifier.go 55.63% <0.00%> (-0.22%) ⬇️
... and 18 more

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

models/unit/unit.go Outdated Show resolved Hide resolved
@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 Apr 3, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Apr 3, 2023
@lunny lunny added this to the 1.20.0 milestone Apr 3, 2023
@wxiaoguang
Copy link
Contributor

I guess there is a related PR

@wolfogre
Copy link
Member

wolfogre commented Apr 3, 2023

Sorry I overlooked this PR and posted #23736.

Since #23736 also handles DisabledRepoUnits, DefaultRepoUnits, and DefaultForkRepoUnits, @yp05327, what do you think about replacing this one?

@yp05327 yp05327 closed this Apr 4, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone Apr 23, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants