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 -noReconnectAfter option support #802

Closed
wants to merge 1 commit into from

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented May 7, 2024

Testing done

Submitter checklist

@jglick
Copy link
Member

jglick commented May 7, 2024

Do we really need this? I feel like we should deprecate the env vars in this launcher script except for one catch-all REMOTING_OPTS var with whatever CLI options you like; then https://github.com/jenkinsci/kubernetes-plugin/blob/f10083a42e708a6bff990c6ccb306ce618b8bccd/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java#L415-L449 could (eventually, once adopted) be simplified to just construct its list of CLI options directly without needing to make further changes to this image beyond routine JAR bumps.

@dduportal
Copy link
Contributor

Do we really need this? I feel like we should deprecate the env vars in this launcher script except for one catch-all REMOTING_OPTS var with whatever CLI options you like; then https://github.com/jenkinsci/kubernetes-plugin/blob/f10083a42e708a6bff990c6ccb306ce618b8bccd/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java#L415-L449 could (eventually, once adopted) be simplified to just construct its list of CLI options directly without needing to make further changes to this image beyond routine JAR bumps.

Good idea! Just be warned that there are other use cases of this launcher script AFAIK such as static agents or docker plugin for example.

@jglick
Copy link
Member

jglick commented May 7, 2024

Yes, and for compatibility we need to continue to support the currently defined variables, I am just suggesting that going forward we deprecate them in favor of a generic launcher options variable. Would also allow us to deprecate and eventually remove messy stuff like

case "$@" in
*"-workDir"*) echo "Warning: Work directory is defined twice in command-line arguments and the environment variable" ;;

docker-agent/jenkins-agent

Lines 131 to 132 in 611dc9d

#TODO: Handle the case when the command-line and Environment variable contain different values.
#It is fine it blows up for now since it should lead to an error anyway.

You can already pass any arguments you like to the entrypoint ("$@") but for purposes of some cloud impls like PodTemplateBuilder it is easier to set environment variables.

@Vlatombe
Copy link
Member Author

I had forgotten about that, let's just close this.

@Vlatombe Vlatombe closed this May 13, 2024
@Vlatombe
Copy link
Member Author

any arguments you like to the entrypoint

Only on linux, the windows launcher script is missing this option.

Filed #809 as a draft.

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

Successfully merging this pull request may close these issues.

3 participants