Docker bootstrap script #213
Conversation
literalplus
force-pushed
the
docker-bootstrap-script
branch
from
1eed5a8
to
f2c00fd
Compare
|
I need to review it carefully. I just expected few params in |
|
The Although the new script doesn't do much more than that, just moves a few repeated paths into environment variables so we don't have to duplicate the final values in both the Dockerfile and the script. I'll resolve the new merge conflicts after the review is in to avoid extra traffic on this PR – the affected files seem to change frequently. |
Yes, I forgot it exists. It would be the best place for the new CLI argument so that we do not create more small scripts. |
|
I can move the logic into that script. Do you know if it is used anywhere else that we would need to stay compatible with? What should it do if the required environment variables (jenkins home, jenkins ref dir) are not defined? Error out like now (breaks existing usages of the script, if any) or only pass the We might be able to save this for I'll probably only get around to doing that end of November |
The script is used only for Docker images. Jenkinsfile Runner is in Beta, hence there is no strict compatibility requirements ATM
Not sure I get it. AFAICT there is no serious changes needed being compared with the current behavior |
not an issue if it's only used for Docker :) I thought it might be used for launching in standalone as well, in that case people with existing setups might call that script without the environment variables set |
This allows users to pass "cli" or "run" as Docker CMD to easily switch modes without having to set environment variables. Also provides the DEBUG environment variable to the non-dev Dockerfile. Removes the FORCE_JENKINS_CLI environment variables as it's no longer necessary. as per jenkinsci#207 (comment)
literalplus
force-pushed
the
docker-bootstrap-script
branch
from
f2c00fd
to
49279c5
Compare
|
Updated to expand the existing script |
|
retriggering CI |
|
Sorry for nt acting on this PR. The Dockerfile patch is still relevant, but I have a slightly different patch in mind: let's modify how CLI works by adding a top-level command. E.g. it would be:
It would make the CLI more extensible in the future, and it will help to simplify the Dockerfile as well |
|
That sounds like a good idea! I assume we'd want these commands handled in the app itself instead of the wrapper script then. Thus I'd remove the top-level command handling from the launcher (lines 28-42). However this doesn't make the Dockerfile any simpler than it already is in this PR, so am I understanding this correctly? I think it makes sense to do refactoring the CLI as a separate task/PR, since it is technically independent from these changes here, which we could then merge for now (minus lines 28-42). I could move the CLI logic into the app for this year's Hacktoberfest if that works for you. Having top-level commands, we might also be able to split their logic into separate classes, making Bootstrap.java a little slimmer. (I haven't looked at args4j in detail yet, I hope it supports that.) |
|
Sorry for the ping @oleg-nenashev, do you have feedback on what I suggested in my previous comment? Would be great to have this cleared up before Hacktoberfest starts if we want to do it with that :) BTW I'm available to do other stuff as well so if you already have anything specific in mind that needs to be done besides the issues that are already tagged, I'd be happy to help! re: making it simpler - you're right, we could save the case statement etc. and just always pass -f and --runWorkspace with your suggestion. (= lines 28-42 from above). If the above is fine, I'll rebase this PR (otherwise we could also drop this one and do these changes together with a new top-level command PR) |
|
Hi @literalplus . My apologies, I missed the notification.
Yes. It is rather a longer-term change when more commands are introduced
args4j supports only one-level CLI, so it will require manual implementation. Anyway, it is quite easy to implement multiple option classes with inheritance where needed
Any contributions are welcome! Right now I am mostly focused on getting the new packaging flow in place + on performance/size optimizations. But there is a lot of opportunities in the code. https://github.com/jenkinsci/jenkinsfile-runner-image-packs is also an area where many contributions could be made (see CONTRIBUTING) |
|
Since this just popped up in my notifications, a short heads-up: I was unable to participate in this year's Hacktoberfest due to private issues, sorry for not reporting back. I can't yet say when I'll be able to reallocate time for this yet, but will ✨ eventually ✨ be able to take a look (but it's totally fine if it's done by then, there will be other things to do in the future) |
|
Sure @literalplus . Any contributions are always appreciated and welcome, regardless of Hacktoberfest. I have started implementing #429 , it should be done this week. But there is a lot of other stuff that could be done |
Ellipsis
This MR adds a bootstrap script for Docker, which allows us to switch between Jenkinsfile and CLI mode seamlessly without having to mess with environment variables. It also streamlines a few operations in the Dockerfiles to use environment variables.
This is as suggested by @oleg-nenashev in #207 (comment), and inherently requires that #207 (CLI mode) be merged first.
Clean diff without the #207 changes here: https://github.com/xxyy/jenkinsfile-runner/compare/178/cli-mode...xxyy:docker-bootstrap-script?expand=1