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

Move docker resource limit settings from server to agent #3174

Merged

Conversation

6543
Copy link
Member

@6543 6543 commented Jan 11, 2024

so you can set it per agent and not per server

related to #3156 to cleanup the step definition

so you can set it per agent and not per server
@6543 6543 added the enhancement improve existing features label Jan 11, 2024
@lafriks
Copy link
Contributor

lafriks commented Jan 11, 2024

Maybe server can still provide defaults?

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 42 lines in your changes missing coverage. Please review.

Project coverage is 26.54%. Comparing base (fcc57df) to head (68288ae).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pipeline/backend/docker/config.go 0.00% 27 Missing ⚠️
pipeline/backend/docker/docker.go 0.00% 8 Missing ⚠️
pipeline/backend/docker/convert.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3174      +/-   ##
==========================================
- Coverage   26.60%   26.54%   -0.07%     
==========================================
  Files         373      374       +1     
  Lines       27081    27045      -36     
==========================================
- Hits         7205     7178      -27     
+ Misses      19213    19210       -3     
+ Partials      663      657       -6     
Flag Coverage Δ
26.54% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@6543
Copy link
Member Author

6543 commented Jan 11, 2024

no I'm against this ...

we for example also do not set the tmp path for the local backend on the server side

@6543 6543 requested a review from lafriks January 11, 2024 21:24
@lafriks lafriks added the breaking will break existing installations if no manual action happens label Jan 11, 2024
@lafriks
Copy link
Contributor

lafriks commented Jan 11, 2024

I have no strong opinion about this but that would avoid it being breaking change

@6543
Copy link
Member Author

6543 commented Jan 12, 2024

well so it looks like v3.0.0 is right around the corner

@anbraten
Copy link
Member

I think it the default fits well into the agent settings, specific settings however should probably be still settable via the pipeline config. We just need to move it into the backend config section.

@6543
Copy link
Member Author

6543 commented Jan 12, 2024

well make it non breaking for now then ... but add a TODO comment so we can do so on next majour bump

also I add a docker specific config like with kubernetis for limiting per step

@6543 6543 added the wip label Jan 12, 2024
@qwerty287 qwerty287 marked this pull request as draft January 21, 2024 08:06
@qwerty287 qwerty287 added this to the 3.0.0 milestone Jul 17, 2024
@6543 6543 mentioned this pull request Jul 19, 2024
@6543 6543 marked this pull request as ready for review July 22, 2024 20:01
@6543 6543 removed the wip label Jul 22, 2024
@6543 6543 requested a review from a team July 22, 2024 20:03
@woodpecker-bot
Copy link
Contributor

woodpecker-bot commented Jul 22, 2024

docs/docs/91-migrations.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

Fits better into the agent than server but I agree, it should be settable in the step config like for k8s, as every container usually has a different requirement.

Given it's breaking due to the move, it fits well into 3.0.

@6543
Copy link
Member Author

6543 commented Sep 26, 2024

yes have it alser per stes should be an backend option like done for kubes settings. this will be a new issue to address ...

@6543 6543 merged commit 6ad20ce into woodpecker-ci:main Sep 26, 2024
7 of 9 checks passed
@6543 6543 deleted the remove_docker-cpu-config-from-frontend branch September 26, 2024 15:57
@woodpecker-bot woodpecker-bot mentioned this pull request Sep 26, 2024
1 task
@woodpecker-bot woodpecker-bot mentioned this pull request Dec 14, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking will break existing installations if no manual action happens enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants