Skip to content

Refactor secrets modification logic #26873

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 5, 2023
Merged

Refactor secrets modification logic #26873

merged 6 commits into from
Sep 5, 2023
+344 −204

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Sep 1, 2023

  • Share code between web and api
  • Add some tests

@KN4CK3R KN4CK3R added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Sep 1, 2023
@KN4CK3R KN4CK3R added this to the 1.21.0 milestone Sep 1, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 1, 2023
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Sep 1, 2023
KN4CK3R
KN4CK3R commented Sep 1, 2023
View reviewed changes
routers/api/v1/org/action.go
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/routers/api/v1/utils"
"code.gitea.io/gitea/routers/web/shared/actions"
secret_service "code.gitea.io/gitea/services/secrets"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use 007 as import name 😢

@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Sep 2, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 2, 2023 •
edited
Loading

It seems inconsistency about name handling.

For example, create a "lower_case_secret", it's stored as "LOWER_CASE_SECRET" in database, then could it be deleted by "lower_case_secret"? Some database strings are case-sensitive by default.

OK, I can see that this case is handled by FindSecrets

wxiaoguang
wxiaoguang approved these changes Sep 2, 2023
View reviewed changes
services/secrets/secrets.go
return secret_model.ErrSecretNotFound{}
}

return deleteSecret(ctx, s[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, why not deleteSecret(ctx, s[0].ID) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had the audit PR in my mind because I need the full object there. deleteSecret would be the method where the log is created.

@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 Sep 2, 2023
@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 Sep 4, 2023
lunny
lunny reviewed Sep 5, 2023
View reviewed changes
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Sep 5, 2023
@KN4CK3R KN4CK3R enabled auto-merge (squash) September 5, 2023 14:27
@KN4CK3R KN4CK3R added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 5, 2023
@KN4CK3R KN4CK3R mentioned this pull request Sep 5, 2023
Open
@KN4CK3R KN4CK3R merged commit a99b96c into go-gitea:main Sep 5, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 5, 2023
@KN4CK3R KN4CK3R deleted the refactor-secrets branch September 5, 2023 18:34
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 6, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
dc4aa3f 
* giteaofficial/main:
  Add missing translation (go-gitea#26926)
  Vendor `jquery.are-you-sure` with strict mode fixes (go-gitea#26901)
  Fix the secret regexp pattern on web page (go-gitea#26910)
  Add @chenrui333 as maintainer (go-gitea#26917)
  Move notification interface to services layer (go-gitea#26915)
  fetch emails of currently displayed user on admin page (go-gitea#26918)
  Improve LDAP group config documentation, fixes go-gitea#21159 (go-gitea#21227)
  update footer link to new landing page (go-gitea#26916)
  Remove `Named` interface (go-gitea#26913)
  Refactor secrets modification logic (go-gitea#26873)
  Add missing `reqToken()` to notifications endpoints (go-gitea#26914)
  feat(API): add routes and functions for managing user's secrets (go-gitea#26909)
  Move feed notification service layer (go-gitea#26908)
  Extract common code to new template (go-gitea#26903)
  Move ui notification to service layer (go-gitea#26907)
  Remove duplicated notify mail configuration on tests (go-gitea#26912)
  Move indexer notification to service layer (go-gitea#26906)

# Conflicts:
#	templates/base/footer_content.tmpl
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang

@lunny lunny

@silverwind silverwind

Assignees
No one assigned
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
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@KN4CK3R @wxiaoguang @lunny @silverwind @GiteaBot