jenkinsci / docker-agent

Jenkins agent (base image) and inbound agent Docker images
https://hub.docker.com/r/jenkins/inbound-agent/
MIT License
282 stars 232 forks source link

Add `-noReconnectAfter` option support #802

Closed Vlatombe closed 5 months ago

Vlatombe commented 5 months ago

Testing done

### Submitter checklist
- [ ] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main 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
jglick commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 https://github.com/jenkinsci/docker-agent/blob/611dc9df849eee9334a3351a6a520e2106116dc3/jenkins-agent#L60-L61 https://github.com/jenkinsci/docker-agent/blob/611dc9df849eee9334a3351a6a520e2106116dc3/jenkins-agent#L131-L132

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 commented 5 months ago

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

Vlatombe commented 5 months ago

any arguments you like to the entrypoint

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

Filed https://github.com/jenkinsci/docker-agent/pull/809 as a draft.