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 DeletePipeline API #3506

Merged
merged 14 commits into from
Apr 25, 2024
Merged

Add DeletePipeline API #3506

merged 14 commits into from
Apr 25, 2024

Conversation

xoxys
Copy link
Member

@xoxys xoxys commented Mar 18, 2024

This is just a first step, the final goal is to have an API endpoint to prune Repo Pipelines older than the given date.

@woodpecker-ci/maintainers Can I get some feedback if this is the right direction?

@xoxys xoxys marked this pull request as draft March 18, 2024 21:13
@6543 6543 added server enhancement improve existing features labels Mar 18, 2024
@6543 6543 added this to the 2.5.0 milestone Mar 18, 2024
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Mar 19, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-3506.surge.sh

server/api/pipeline.go Outdated Show resolved Hide resolved
server/model/pipeline.go Outdated Show resolved Hide resolved
@xoxys
Copy link
Member Author

xoxys commented Mar 19, 2024

Will add some more tests for the delete API later today.

@xoxys xoxys changed the title Add filter options to GetPipelines Add filter options to GetPipelines and add DeletePipeline API Mar 19, 2024
@xoxys xoxys changed the title Add filter options to GetPipelines and add DeletePipeline API Add filter options to GetPipelines and add DeletePipelines API Mar 19, 2024
@xoxys xoxys marked this pull request as ready for review March 19, 2024 21:26
@xoxys xoxys requested a review from a team March 19, 2024 21:26
@xoxys
Copy link
Member Author

xoxys commented Mar 19, 2024

Should we prevent deleting active pipelines through the exposed api.DeletePipelines?

@6543
Copy link
Member

6543 commented Mar 20, 2024

☝️ yes & pending ... as they have to be removed from task queue

server/router/api.go Outdated Show resolved Hide resolved
@xoxys
Copy link
Member Author

xoxys commented Mar 20, 2024

☝️ yes & pending ... as they have to be removed from task queue

I have added a helper function to check the status and applied it to DeletePipelineLogs as well. model.StatusRunning, model.StatusPending, model.StatusBlocked are blocked now, for both delete actions.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 48.10127% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 36.08%. Comparing base (326069c) to head (b92012b).
Report is 12 commits behind head on main.

❗ Current head b92012b differs from pull request most recent head f901408. Consider uploading reports for the commit f901408 to get more accurate results

Files Patch % Lines
server/api/pipeline.go 39.34% 33 Missing and 4 partials ⚠️
server/store/datastore/pipeline.go 75.00% 2 Missing and 1 partial ⚠️
server/api/badge.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3506      +/-   ##
==========================================
+ Coverage   36.04%   36.08%   +0.03%     
==========================================
  Files         229      229              
  Lines       15493    15565      +72     
==========================================
+ Hits         5585     5616      +31     
- Misses       9502     9538      +36     
- Partials      406      411       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anbraten
Copy link
Member

@xoxys Is your plan to have something like requested in #1068? (seems to have quite a few 👍🏾) So currently this would be done by calling the api, should we add some cron / ui button as well?

@xoxys
Copy link
Member Author

xoxys commented Mar 21, 2024

Is your plan to have something like requested in #1068

In general, yes. My DB is growing constantly already.

should we add some cron / ui button as well?

I would like to separate this. First, let's get the API changes merged. After that, we can add UI components, CLI, etc.
Personally, I don't need a UI, I will just use the CLI to run a scheduled job to clean up old pipelines.

Note: These APIs are about deleting the entire pipeline, not only it logs.

@xoxys
Copy link
Member Author

xoxys commented Mar 21, 2024

In addition to this, an API DeletePipelineLogs exists already and there is a PR #3451 to add the ability to delete logs of particular steps.

@anbraten
Copy link
Member

In general, yes. My DB is growing constantly already.

I think the same would be of value for the woodpecker project (and probably most others) as well. We normally care about the last 100 pipelines, but I can't imagine looking at pipelines from the last year etc.

@anbraten
Copy link
Member

One think I am wondering is, would it be cleaner to have a delete-pipeline endpoint instead of delete-pipelines (as that could be used to delete a single pipeline) similar to delete pipeline-logs of a single pipeline etc. And then the get endpoint with time filters could be used to get all pipelines before and after x and delete them with a script etc by using their id?

@xoxys
Copy link
Member Author

xoxys commented Mar 21, 2024

I would like to avoid extra scripting around it. However, I can also add DeletePipeline API that just calls store.DeletePipeline to delete a single pipeline by ID. Or is there any downside to have both API endpoints?

@xoxys xoxys requested a review from a team March 21, 2024 09:51
@xoxys
Copy link
Member Author

xoxys commented Mar 24, 2024

@woodpecker-ci/maintainers Anything missing for this?

@anbraten
Copy link
Member

I would like to avoid extra scripting around it.

I see, but shouldn't we then integrate a cron job doing that based on a setting into wp directly then?

However, I can also add DeletePipeline API that just calls store.DeletePipeline to delete a single pipeline by ID. Or is there any downside to have both API endpoints?

It feels quite powerful to me having an endpoint that could delete all pipelines of a repo.

@xoxys
Copy link
Member Author

xoxys commented Mar 24, 2024

It feels quite powerful to me having an endpoint that could delete all pipelines of a repo.

Please make a decision and I'll implement it that way then.

I see, but shouldn't we then integrate a cron job doing that based on a setting into wp directly then?

Again, yes we can add client integrations (UI, CLI) later please let's get the API changes merged first.

@xoxys xoxys changed the title Add filter options to GetPipelines and add DeletePipeline API Add DeletePipeline API Apr 25, 2024
server/api/helper.go Outdated Show resolved Hide resolved
Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

Didn't test, but seems fine

@xoxys xoxys merged commit d005773 into main Apr 25, 2024
6 of 7 checks passed
@xoxys xoxys deleted the pipeline-filters branch April 25, 2024 08:59
@xoxys xoxys modified the milestones: 2.6.0, 2.5.0 Apr 25, 2024
@woodpecker-bot woodpecker-bot mentioned this pull request Apr 25, 2024
1 task
@anbraten anbraten added feature add new functionality and removed enhancement improve existing features labels May 6, 2024
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
Separate this change from
woodpecker-ci#3506

I would like to get at least this change into v2.5.0 if possible.

---------

Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com>
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
This is just a first step, the final goal is to have an API endpoint to
prune Repo Pipelines older than the given date.

@woodpecker-ci/maintainers Can I get some feedback if this is the right
direction?

---------

Co-authored-by: 6543 <m.huber@kithara.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature add new functionality server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants