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

Rework entrypoints #3269

Merged
merged 22 commits into from
May 2, 2024
Merged

Rework entrypoints #3269

merged 22 commits into from
May 2, 2024

Conversation

qwerty287
Copy link
Contributor

@qwerty287 qwerty287 commented Jan 24, 2024

Closes #3263
Closes #3477

@qwerty287 qwerty287 added refactor delete or replace old code pipeline-config related to pipeline config files (.yaml) labels Jan 24, 2024
@qwerty287 qwerty287 requested a review from a team January 24, 2024 14:58
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Jan 24, 2024

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

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

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

Project coverage is 35.90%. Comparing base (d494b6a) to head (060c713).
Report is 27 commits behind head on main.

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

Files Patch % Lines
pipeline/backend/docker/convert.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3269      +/-   ##
==========================================
- Coverage   35.91%   35.90%   -0.02%     
==========================================
  Files         233      233              
  Lines       15670    15667       -3     
==========================================
- Hits         5628     5625       -3     
  Misses       9619     9619              
  Partials      423      423              

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

@zc-devs
Copy link
Contributor

zc-devs commented Jan 24, 2024

LGTM. Build image, please, I'll test in Kubernetes.

@qwerty287 qwerty287 added the build_pr_images If set, the CI will build images for this PR and push to Dockerhub label Jan 24, 2024
@qwerty287
Copy link
Contributor Author

LGTM. Build image, please, I'll test in Kubernetes.

Will be done when I push next time (probably the next merge from main).

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

I'm also against adding another config flat

@6543
Copy link
Member

6543 commented Jan 25, 2024

what about, if the custom entrypint is set, thread comments as arguments.

this way you can make both usages work:

entrypoint:
  - /usr/bin/curl
 commands:
  - "-v"
  - "--insecure"
  - https://google.com
entrypoint:
  - /bin/bash
 commands:
  - "-x"
  - "-e"
  - "-c 'echo Hello'; printenv"

#3263 (reply in thread)

@zc-devs
Copy link
Contributor

zc-devs commented Jan 25, 2024

if the custom entrypint is set, thread comments as arguments

That's an option. What's about "magic" aspect then?

Also, we have to find and provide entrypoint for each "scratch" image. We cannot use default image's entrypoint:

steps:
  build:
    image: gcr.io/kaniko-project/executor:v1.18.0
    args:
      - --context
      - ./
      - --destination
      - harbor.domain.net/project/image:123

Anyway, this would be better than nothing current state.

@6543
Copy link
Member

6543 commented Jan 25, 2024

☝️ should my soulution not work in this way also ?!?

@qwerty287
Copy link
Contributor Author

what about, if the custom entrypint is set, thread comments as arguments.

In general, this should work, but I don't like it.

Commands are commands and not arguments. If we change this behavior, it wouldn't be possible anymore to use a different shell as entrypoint. And, if we change it there's also a completely different handling of commands: if using without entrypoint, the commands are executed one after each other, otherwise they're executed at the same (all as args of one command).

Probably the only way to fix this is to change how the backends work, there shouldn't be a script anymore. In this case, this could be implemented in a similar way.

@anbraten
Copy link
Member

there shouldn't be a script anymore. In this case, this could be implemented in a similar way.

We / I had a look at that approach some time ago as well, but actually stopped as there seems to be no smart way of doing this. My first idea was to simply execute commands one by one using sth like docker exec ..., but that doesn't work as vars or the current directory wouldn't work anymore. The other idea I had was using screen / tmux, but that would require even more specific images to be used.

@qwerty287
Copy link
Contributor Author

@woodpecker-ci/maintainers @zc-devs Would it be possible to limit entrypoint to the first element of the entrypoint YAML slice?

I.e. if you have:

entrypoint:
- /bin/some-bin
- --some-arg
- ./
- --some-flag

set /bin/some-bin as entrypoint and [--some-arg, ./, --some-flag] as arguments.

@zc-devs
Copy link
Contributor

zc-devs commented Mar 24, 2024

Would it be possible to limit entrypoint to the first element of the entrypoint YAML slice?

Of course, yes.

  1. We have to find and provide entrypoint for each "scratch" image. We cannot use default image's entrypoint. (Rework entrypoints #3269 (comment), How do you use step:entrypoint? #3263 (reply in thread))
  2. Would you allow to use commands with entrypoint? Could you override shell (How do you use step:entrypoint? #3263 (reply in thread), propose, point 1), but using commands as before(script)?
entrypoint:
  - /bin/python
  - "-strict"
 commands:
  - print("Hello")
  - quit()

@qwerty287
Copy link
Contributor Author

Would you allow to use commands with entrypoint? Could you override shell (#3263 (reply in thread), propose, point 1), but using commands as before(script)?

Yes, that would be my idea.
Your example with Python won't work though as the script uses Shell-specific stuff (ex. https://github.com/woodpecker-ci/woodpecker/blob/main/pipeline/backend/common/script_posix.go#L44).

@qwerty287
Copy link
Contributor Author

qwerty287 commented Mar 24, 2024

I updated the complete feature now, please take a look.

@zc-devs Unfortunately, custom shell is currently only possible if you specify the script execution yourself.
Example (that's the default entrypoint):

entrypoint: ["/bin/sh", "-c", "echo $CI_SCRIPT | base64 -d | /bin/sh -e"]

However, I wouldn't try supporting this in this PR, but rather in a new one.

@qwerty287 qwerty287 requested a review from a team March 24, 2024 14:37
@qwerty287 qwerty287 added this to the 2.5.0 milestone Mar 24, 2024
@zc-devs
Copy link
Contributor

zc-devs commented Mar 24, 2024

Seems, doesn't work.

Image: docker.io/woodpeckerci/woodpecker-agent@sha256:25d4d4871ec16155e1cc7991141e277089dc9ec1f0a4922c789fd00863ec59a5

Pipeline:

skip_clone: true
steps:
  server:
    image: quay.io/curl/curl
    entrypoint:
      - /usr/bin/curl
      - -v
      - google.com

Output:

curl: try 'curl --help' or 'curl --manual' for more information

Warnings:

[linter]woodpecker: stepsMust validate one and only one schema (oneOf)
[linter]woodpecker: steps.serverAdditional property entrypoint is not allowed
[bad_habit]Please set an event filter on all when branches

pod-entrypoint-only.yaml

@zc-devs
Copy link
Contributor

zc-devs commented Mar 24, 2024

Fixed_custom_entrypoint.patch

skip_clone: true
steps:
  server:
    image: quay.io/curl/curl
    entrypoint:
      - /usr/bin/curl
      - -v
      - -k
      - https://google.com
 0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0* Host google.com:443 was resolved.
* IPv6: 2a00:1450:4007:80e::200e
...
* using HTTP/2
* [HTTP/2] [1] OPENED stream for https://google.com/
...
* Connection #0 to host google.com left intact
<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>301 Moved</TITLE></HEAD><BODY>
<H1>301 Moved</H1>
The document has moved
<A HREF="https://www.google.com/">here</A>.

pod-entrypoint-only.yaml
woodpecker-agent-entrypoint-only.log

skip_clone: true
steps:
  server:
    image: debian
    entrypoint:
      - /bin/bash
      - -c
      - echo $CI_SCRIPT | base64 -d | /bin/bash -e
    commands:
      - echo $SHELL
      - test=(1 2 4 8 16 32 64 128); echo $${test[@]};
+ echo $SHELL
/bin/sh
+ test=(1 2 4 8 16 32 64 128); echo ${test[@]};
1 2 4 8 16 32 64 128

pod-custom-shell.yaml
woodpecker-agent-custom-shell.log

skip_clone: true
steps:
  server:
    image: debian
    commands:
      - echo $SHELL
      - test=(1 2 4 8 16 32 64 128); echo $${test[@]};
+ echo $SHELL
/bin/sh
+ test=(1 2 4 8 16 32 64 128); echo ${test[@]};
/bin/sh: 18: Syntax error: "(" unexpected

pod-sh.yaml
woodpecker-agent-sh.log

Co-authored-by: Thomas Anderson <127358482+zc-devs@users.noreply.github.com>
@qwerty287
Copy link
Contributor Author

So the reason for requesting changes (#3269 (review)) was the additional config option, that one is resolved, however, the solution in #3269 (comment) is not how it's done now as entrypoint and arguments are in one config option now… So I can't tell you

@anbraten
Copy link
Member

So the reason for requesting changes (#3269 (review)) was the additional config option, that one is resolved, however, the solution in #3269 (comment) is not how it's done now as entrypoint and arguments are in one config option now… So I can't tell you

In that case it would be fair to wait for the approval.

@qwerty287
Copy link
Contributor Author

@6543 can you take a look here? You requested changes so we cant merge without your approval

@qwerty287 qwerty287 modified the milestones: 2.5.0, 2.6.0 Apr 19, 2024
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

this will make it so we can not pass arguments ... but It's probably the best we can do as of now :/

@qwerty287 qwerty287 merged commit 225ddb5 into woodpecker-ci:main May 2, 2024
6 of 7 checks passed
@qwerty287 qwerty287 deleted the rework-entry branch May 2, 2024 12:52
@qwerty287 qwerty287 modified the milestones: 2.6.0, 2.5.0 May 2, 2024
@woodpecker-bot woodpecker-bot mentioned this pull request May 2, 2024
1 task
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
Co-authored-by: Thomas Anderson <127358482+zc-devs@users.noreply.github.com>
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
build_pr_images If set, the CI will build images for this PR and push to Dockerhub pipeline-config related to pipeline config files (.yaml) refactor delete or replace old code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overriding 'entrypoint' in kubernetes backend does not work
5 participants