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.

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

Skip pipeline if config not found #2313

Closed
wants to merge 9 commits into from
Closed

Skip pipeline if config not found #2313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6fe0d5a
Skip pipeline if config not found
xoxys Aug 21, 2023
cd6202e
use custom error to check for missing pipeline config
xoxys Aug 23, 2023
5d62e31
Merge branch 'main' into skip-missing
xoxys Aug 23, 2023
8a5bb4b
Merge branch 'main' into skip-missing
xoxys Aug 30, 2023
b7b19a8
add note to migration docs
xoxys Sep 8, 2023
50bc924
Merge branch 'main' into skip-missing
xoxys Sep 8, 2023
92f54e7
Merge branch 'main' into skip-missing
xoxys Sep 8, 2023
e36d204
Merge branch 'main' into skip-missing
xoxys Sep 8, 2023
5f64e20
Merge branch 'main' into skip-missing
xoxys Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/docs/91-migrations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Some versions need some changes to the server configuration or the pipeline conf
- Drop deprecated `pipeline:` keyword for steps in yaml config
- Drop deprecated `branches:` keyword for global branch filter
- Deprecate `platform:` filter in favor of `labels:`, [read more](./20-usage/20-pipeline-syntax.md#filter-by-platform)
- Ignore branches without a Woodpecker configuration

## 1.0.0

Expand Down
2 changes: 2 additions & 0 deletions server/api/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
func handlePipelineErr(c *gin.Context, err error) {
if errors.Is(err, &pipeline.ErrNotFound{}) {
c.String(http.StatusNotFound, "%s", err)
} else if errors.Is(err, &pipeline.ErrConfigNotFound{}) {
c.String(http.StatusBadRequest, "%s", err)
} else if errors.Is(err, &pipeline.ErrBadRequest{}) {
c.String(http.StatusBadRequest, "%s", err)
} else if errors.Is(err, &pipeline.ErrFiltered{}) {
Expand Down
3 changes: 3 additions & 0 deletions server/api/hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@ func PostHook(c *gin.Context) {
//

pl, err := pipeline.Create(c, _store, repo, tmpPipeline)
if errors.Is(err, pipeline.ErrConfigNotFound{}) {
return
}
if err != nil {
handlePipelineErr(c, err)
} else {
Expand Down
4 changes: 3 additions & 1 deletion server/pipeline/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,12 @@ func Create(ctx context.Context, _store store.Store, repo *model.Repo, pipeline

if configFetchErr != nil {
log.Debug().Str("repo", repo.FullName).Err(configFetchErr).Msgf("cannot find config '%s' in '%s' with user: '%s'", repo.Config, pipeline.Ref, repoUser.Login)
err := &ErrConfigNotFound{Msg: fmt.Sprintf("pipeline definition not found in %s", repo.FullName)}
pipeline.Started = time.Now().Unix()
pipeline.Finished = pipeline.Started
pipeline.Status = model.StatusError
pipeline.Error = fmt.Sprintf("pipeline definition not found in %s", repo.FullName)
pipeline.Error = err.Msg
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just add?

Suggested change
return nil, err
if pipeline.Branch != repo.Branch {
return nil, err
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not just let the user control it? Why should we assume that the default branch requires a CI config for every user or setup? We can add options to disable/enable this feature later right now irgnoring it and logging an error in the backend is good enough IMO.

Copy link
Contributor

@lafriks lafriks Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not for my use case, this will increase our support team work on finding out why pipelines are not working for new projects because even with current error that config was not found there were some developers that asked for help. Now it will be even harder to find that as developers won't see anything and it will just not work silently.
You are currently taking away what I need to make it how you need. Imho my proposed solution would work for both our needs

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand your point but I still disagree. However not my decision, Ill leave this PR to someone else to pick up or make a decision.

Copy link
Contributor

Choose a reason for hiding this comment

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

But from what you wrote previously this would work also for you so this would be good middle ground that would be good and not too breaking change and later on as you said this could be improved for other cases if needed

} else if parseErr != nil {
log.Debug().Str("repo", repo.FullName).Err(parseErr).Msg("failed to parse yaml")
pipeline.Started = time.Now().Unix()
Expand Down
16 changes: 16 additions & 0 deletions server/pipeline/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,19 @@ func (e *ErrFiltered) Is(target error) bool {
}
return ok
}

type ErrConfigNotFound struct {
Msg string
}

func (e ErrConfigNotFound) Error() string {
return e.Msg
}

func (e ErrConfigNotFound) Is(target error) bool {
_, ok := target.(ErrConfigNotFound) //nolint:errorlint
if !ok {
_, ok = target.(*ErrConfigNotFound) //nolint:errorlint
}
return ok
}