-
Notifications
You must be signed in to change notification settings - Fork 29
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
refactor(repo)!: nested repository with migration from types #1095
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1095 +/- ##
==========================================
+ Coverage 61.93% 64.47% +2.53%
==========================================
Files 358 370 +12
Lines 10901 11788 +887
==========================================
+ Hits 6752 7600 +848
- Misses 3663 3671 +8
- Partials 486 517 +31
|
Putting in draft right now. Functionally it's there, but I'd like to work out more comments, address some tests, and search for more optimizations |
@@ -69,7 +69,7 @@ import ( | |||
// expand and validate a pipeline configuration. | |||
func ValidatePipeline(c *gin.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [golangci] reported by reviewdog 🐶
70-109 lines are duplicate of api/pipeline/compile.go:72-111
(dupl)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -7,11 +7,10 @@ import ( | |||
"fmt" | |||
"net/http" | |||
|
|||
"github.com/go-vela/server/api/types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we move everything to this pattern, any thoughts on giving every types import the api
alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was operating on the following pattern:
go-vela/server/api
—> typesanything else
—> api
But I'm open to using api
always if people would prefer that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool that works for me, not blocking either way
"github.com/go-vela/types/constants" | ||
"github.com/go-vela/types/database" | ||
"github.com/go-vela/types/library" | ||
"github.com/sirupsen/logrus" | ||
) | ||
|
||
// GetDeploymentForRepo gets a deployment by repo ID and number from the database. | ||
func (e *engine) GetDeploymentForRepo(ctx context.Context, r *library.Repo, number int64) (*library.Deployment, error) { | ||
func (e *engine) GetDeploymentForRepo(ctx context.Context, r *api.Repo, number int64) (*library.Deployment, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [golangci] reported by reviewdog 🐶
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
"github.com/sirupsen/logrus" | ||
) | ||
|
||
// CountDeploymentsForRepo gets the count of deployments by repo ID from the database. | ||
func (e *engine) CountDeploymentsForRepo(ctx context.Context, r *library.Repo) (int64, error) { | ||
func (e *engine) CountDeploymentsForRepo(ctx context.Context, r *api.Repo) (int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [golangci] reported by reviewdog 🐶
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
"github.com/go-vela/types/constants" | ||
"github.com/go-vela/types/database" | ||
"github.com/go-vela/types/library" | ||
"github.com/sirupsen/logrus" | ||
) | ||
|
||
// ListDeploymentsForRepo gets a list of deployments by repo ID from the database. | ||
func (e *engine) ListDeploymentsForRepo(ctx context.Context, r *library.Repo, page, perPage int) ([]*library.Deployment, error) { | ||
func (e *engine) ListDeploymentsForRepo(ctx context.Context, r *api.Repo, page, perPage int) ([]*library.Deployment, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [golangci] reported by reviewdog 🐶
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good overall.. a few minor things.
any thoughts around moving some of the "internal" types that live in /internal
now to /internal/types/
or another location?
Continuation of:
Context:
go-vela/community#639