Skip to content

Conversation

@jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented Jan 6, 2023

Based off of #615

This change continues the refactor efforts initially introduced in the above PR.

This adds a new repo package to the github.com/go-vela/server/api package.

This contains all the same handlers that existed previously but with each of them within their own file.

Also, this change skips checking a user's access for orgs, repos, and teams if that resource is in their personal org.

i.e. Vela no longer check's my access level for those resources when interacting with https://github.com/jbrockopp

This will help improve the user experience when developing locally and reduce unneeded calls to the SCM.

@jbrockopp jbrockopp self-assigned this Jan 6, 2023
@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Merging #754 (97b5842) into main (96b5d86) will increase coverage by 2.02%.
The diff coverage is 38.63%.

❗ Current head 97b5842 differs from pull request most recent head 7f427d0. Consider uploading reports for the commit 7f427d0 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
+ Coverage   54.87%   56.89%   +2.02%     
==========================================
  Files         244      243       -1     
  Lines       16506    15939     -567     
==========================================
+ Hits         9058     9069      +11     
+ Misses       7048     6468     -580     
- Partials      400      402       +2     
Impacted Files Coverage Δ
router/middleware/perm/perm.go 60.80% <10.00%> (-1.41%) ⬇️
scm/github/access.go 82.17% <33.33%> (-13.12%) ⬇️
scm/github/repo.go 75.00% <100.00%> (+0.44%) ⬆️

@jbrockopp jbrockopp marked this pull request as ready for review March 4, 2023 17:41
@jbrockopp jbrockopp requested a review from a team as a code owner March 4, 2023 17:41
cognifloyd
cognifloyd previously approved these changes Mar 9, 2023
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

One minor nit, but looks good to me anyway. It's nice to see all of this moving into separate modules with shorter files. So much nicer.

cognifloyd
cognifloyd previously approved these changes Mar 10, 2023
ecrupper
ecrupper previously approved these changes Mar 15, 2023
Copy link
Contributor

@ecrupper ecrupper left a comment

Choose a reason for hiding this comment

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

A couple questions out of curiosity, but otherwise LGTM — I like the refactor

@jbrockopp jbrockopp dismissed stale reviews from ecrupper and cognifloyd via cd47c58 March 16, 2023 20:22
@jbrockopp
Copy link
Contributor Author

@cognifloyd @ecrupper ready for review again 👍

@ecrupper ecrupper merged commit b42e80f into main Mar 21, 2023
@ecrupper ecrupper deleted the refactor/api/repo branch March 21, 2023 02:05
@jbrockopp jbrockopp added the enhancement Indicates an improvement to a feature label Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Indicates an improvement to a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants