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 default value for command & args in README document #993

Merged
merged 1 commit into from
May 18, 2021

Conversation

cybernagle
Copy link
Contributor

add default value for command & args, command is sleep, and args default value is 99999999.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@cybernagle cybernagle changed the title command default value is , and args default value is add default value for command & args in README document May 17, 2021
README.md Outdated Show resolved Hide resolved
@cybernagle cybernagle force-pushed the update_command_default_value branch 2 times, most recently from fbe2af2 to 6e6e12d Compare May 18, 2021 05:35
@cybernagle cybernagle force-pushed the update_command_default_value branch from 6e6e12d to 5ed4683 Compare May 18, 2021 05:36
@Vlatombe Vlatombe merged commit fb1e677 into jenkinsci:master May 18, 2021
@jglick
Copy link
Member

jglick commented Jun 2, 2021

I do not think this is really true. These are the suggested values. We decided not to actually set these as defaults: #915 (comment)

@cybernagle
Copy link
Contributor Author

I do not think this is really true. These are the suggested values. We decided not to actually set these as defaults: #915 (comment)

But what we are facing is this is not the suggested value.
I've describe this issue under this PR:
#992 (comment)

@cybernagle cybernagle deleted the update_command_default_value branch June 3, 2021 03:41
@jglick
Copy link
Member

jglick commented Jun 3, 2021

@NagleZhang all that #915 changed was suggested GUI defaults as shown in the Pipeline Syntax and global pod template config pages, as well as samples & docs. You need to explicitly select a command and args in YAML or Groovy if you do not wish to use the default container entrypoint. Therefore the text in this PR is incorrect, or at least misleading.

what we are facing is this is not the suggested value

How so? From #992 (comment)

I changed the arguments without change command

which just sounds like a user error: you can set both of these, or neither, but it would rarely make any sense to set only one.

@cybernagle
Copy link
Contributor Author

which just sounds like a user error: you can set both of these, or neither, but it would rarely make any sense to set only one.

Okay , this make sense.
further more question is,
what if we just change command without arguments ? ( I think this a normal usage for docker)

@jglick
Copy link
Member

jglick commented Jun 4, 2021

if we just change command without arguments

It could be legitimate on occasion—just depends on the image. Not likely I think.

There are several situations here:

  • The default image for the jnlp container is already set up with an appropriate entrypoint, running agent stuff. There is no need to change command or arguments.
  • If you use a nondefault image for jnlp you may or may not need to set command and arguments. For example, there is a single-container pod idiom by which you take a non-Jenkins-specific image with some tools, then add a JRE, plus agent.jar (and optionally also a stock agent launcher shell script) copied from the stock image. If the entrypoint is overridden to be Jenkins-specific in a derived image (FROM generic-image), then you would not specify command/args in the pod template; but if you use a truly stock image containing a JRE without derivation, and for example mount the agent JAR and launcher script from a ConfigMap, you would need to point to these mounts in the command/args of the pod template.
  • Many images have no special entrypoint (just bash or the like), or run some sort of CLI command that would exit quickly. For these, you want to specify sleep 99999999: the container should just wait for the Pipeline to use container('…') {sh '…'}, which basically kubectl execs into it. (Yes this is not how containers are supposed to be used, but it is how we can make them work with Jenkins agents.)
  • Some images have an entrypoint which starts some sort of service. These might make sense if for example tests need to connect to a sample database or whatever, and you might never use the container step with them. For such cases there is no need to change command or arguments.

@cybernagle
Copy link
Contributor Author

The default image for the jnlp container is already set up with an appropriate entrypoint, running agent stuff. There is no need to change command or arguments.

Yes, what we are using is https://github.com/jenkinsci/helm-charts.
from the value they provide from this line

the provide default command is null , but arguments do have like args: "${computer.jnlpmac} ${computer.name}"

and this values is used by _helpers.tpl

assume this is the issue from the our helm-chats ?

and , do you suggest we need more specific description for this configuration ?
like we change line

default value is sleep
to
suggested value is sleep
?

@jglick
Copy link
Member

jglick commented Jun 14, 2021

I am not very familiar with that chart but it looks to apply only to the jnlp container, which would not normally need to have its command or args overridden—the image’s default entrypoint should be left alone.

@cybernagle
Copy link
Contributor Author

I am not very familiar with that chart but it looks to apply only to the jnlp container, which would not normally need to have its command or args overridden—the image’s default entrypoint should be left alone.

Yeah, That's what I'm think about as well.

Thanks for your clarify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants