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

Jump to bottom

Move constrain to only have a single command in backend to run to dedicated backends #1032

Merged
merged 22 commits into from
Oct 30, 2022
Merged

Move constrain to only have a single command in backend to run to dedicated backends #1032

merged 22 commits into from
Oct 30, 2022

Conversation

6543
Copy link
Member

@6543 6543 commented Jul 18, 2022

at the moment we compile a script that we can pipe in as single command
this is because of the constrains the docker backend gives us.

so we move it into the docker backend and eventually get rid of it altogether

@6543 6543 added enhancement improve existing features refactor delete or replace old code labels Jul 18, 2022
@6543 6543 added this to the 1.0.0 milestone Jul 18, 2022
@6543 6543 force-pushed the rm-Command-for-Commands branch from e74b33a to ca7c1ef Compare July 18, 2022 19:23
@anbraten
Copy link
Member

Sounds nice. My idea for the docker backend was to run each command step with docker exec instead of using the script and executing it via docker run once. This would allow us to change the entrypoint as well: #278

anbraten
anbraten reviewed Jul 18, 2022
View reviewed changes
pipeline/backend/docker/convert.go Outdated Show resolved Hide resolved
pipeline/frontend/yaml/container.go Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2022 •
edited
Loading

Codecov Report

Base: 45.08% // Head: 44.93% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (c7b4413) compared to base (280d27d).
Patch coverage: 2.12% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
- Coverage   45.08%   44.93%   -0.15%     
==========================================
  Files         138      138              
  Lines        9988     9959      -29     
==========================================
- Hits         4503     4475      -28     
- Misses       5209     5211       +2     
+ Partials      276      273       -3
Impacted Files Coverage Δ
pipeline/backend/common/script.go 0.00% <0.00%> (ø)
pipeline/backend/common/script_posix.go 100.00% <ø> (ø)
pipeline/backend/common/script_win.go 0.00% <ø> (ø)
pipeline/backend/docker/convert.go 8.59% <0.00%> (-0.14%) ⬇️
pipeline/backend/docker/docker.go 0.00% <ø> (ø)
pipeline/backend/kubernetes/pod.go 0.00% <0.00%> (ø)
pipeline/frontend/yaml/compiler/convert.go 0.00% <0.00%> (ø)
pipeline/frontend/yaml/linter/linter.go 74.71% <ø> (+0.42%) ⬆️
pipeline/frontend/yaml/container.go 82.35% <100.00%> (ø)
server/queue/fifo.go 83.13% <0.00%> (-3.62%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@6543
Copy link
Member Author

6543 commented Jul 18, 2022 •
edited
Loading

Sounds nice. My idea for the docker backend was to run each command step with docker exec instead of using the script and executing it via docker run once. This would allow us to change the entrypoint as well: #278

that can be next ... (and yes i had this also in mind ... and less overhead)

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Jul 18, 2022 •
edited
Loading

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

@6543 6543 mentioned this pull request Jul 18, 2022
Merged
@6543 6543 changed the title WIP: move constrain to only have a single command in backend to run to dedicated backends Move constrain to only have a single command in backend to run to dedicated backends Oct 29, 2022
@6543 6543 marked this pull request as ready for review October 29, 2022 18:41
@6543 6543 requested a review from a team October 29, 2022 18:53
@6543 6543 mentioned this pull request Oct 29, 2022
Closed
3 tasks
This was referenced Oct 29, 2022
Open
Closed
@gapodo
Copy link
Contributor

gapodo commented Oct 29, 2022

After the changes put in via 30f113f it's working on Kubernetes with the Kubernetes backend active
image
https://test-ci.kle.li/sandbox/sq-test/pipeline/10/3
thanks again for the patience while I was stumbling through the setup

@6543
Copy link
Member Author

6543 commented Oct 29, 2022

ok now it's just a refactor and do not change any behavior at all

anbraten
anbraten reviewed Oct 30, 2022
View reviewed changes
pipeline/backend/kubernetes/pod.go Show resolved Hide resolved
pipeline/backend/common/script.go Outdated Show resolved Hide resolved
@6543 6543 merged commit b15ca52 into woodpecker-ci:master Oct 30, 2022
@6543 6543 deleted the rm-Command-for-Commands branch October 30, 2022 23:26
@qwerty287 qwerty287 mentioned this pull request Nov 5, 2022
Merged
simmstein pushed a commit to simmstein/woodpecker that referenced this pull request Dec 27, 2022
@6543 @simmstein
Move constrain to only have a single command in backend to run to ded…
074771b 
…icated backends (woodpecker-ci#1032)

at the moment we compile a script that we can pipe in as single command
this is because of the constrains the docker backend gives us.

so we move it into the docker backend and eventually get rid of it altogether
@6543 6543 mentioned this pull request Jul 28, 2023
Closed
5 tasks
@6543 6543 added the breaking will break existing installations if no manual action happens label Jul 28, 2023
@6543 6543 mentioned this pull request Jul 28, 2023
Merged
6543 added a commit that referenced this pull request Jul 29, 2023
@6543
Add breaking note for command: removal (#2056)
960a52b 
close  #2054

as we missed at #1032 that it was mentioned at one point in the docs and
so it was a breaking change
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Jul 29, 2023
@6543
Add breaking note for command: removal (woodpecker-ci#2056)
e8e8e22 
close  woodpecker-ci#2054

as we missed at woodpecker-ci#1032 that it was mentioned at one point in the docs and
so it was a breaking change
mzampetakis pushed a commit to mzampetakis/woodpecker that referenced this pull request Jul 31, 2023
@6543 @mzampetakis
Add breaking note for command: removal (woodpecker-ci#2056)
0c9ddf7 
close  woodpecker-ci#2054

as we missed at woodpecker-ci#1032 that it was mentioned at one point in the docs and
so it was a breaking change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anbraten anbraten anbraten approved these changes

Assignees
No one assigned
Labels
breaking will break existing installations if no manual action happens enhancement improve existing features refactor delete or replace old code
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@6543 @anbraten @codecov-commenter @woodpecker-bot @gapodo