Combine deploy enhancements #1694
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1694 +/- ##
=======================================
Coverage 52.21% 52.21%
=======================================
Files 275 275
Lines 8498 8498
Branches 616 616
=======================================
Hits 4437 4437
Misses 3568 3568
Partials 493 493
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, all commit messages.
Reviewable status: 1 of 7 files reviewed, 5 unresolved discussions (waiting on @jmgrady)
deploy/Dockerfile line 39 at r1 (raw file):
# Copy configuration management scripts COPY docker_home/* ${HOME}/
Extra newline after this one, but not between the others?
deploy/docker_home/.bashrc line 45 at r1 (raw file):
# off by default to not distract the user: the focus in a terminal window # should be on the output of commands, not on the prompt #force_color_prompt=yes
Is the color_prompt stuff useful to keep around?
deploy/docker_home/restore_config.sh line 3 at r1 (raw file):
# restore /etc/hosts entries for combine targets if [ -f "/config/hosts" ] ; then cp /config/hosts /etc/hosts
Is the /hosts necessary in /etc/hosts?
deploy/scripts/check_certs.py line 73 at r1 (raw file):
["kubectl"] + kubectl_opts + ["get", "secrets", "--field-selector", "type=kubernetes.io/tls", "-o", "name"]
If this array is short enough to fit on one line, might as well do the same for two shorter arrays earlier in the script.
deploy/scripts/setup_target.py line 71 at r1 (raw file):
def main() -> None: """Setup access to the the target specified on the command line."""
"the the"
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 5 unresolved discussions (waiting on @imnasnainaec and @jmgrady)
deploy/Dockerfile line 39 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Extra newline after this one, but not between the others?
Added newlines after the others - made it more readable (at least to me).
deploy/docker_home/.bashrc line 45 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Is the color_prompt stuff useful to keep around?
Yes, for 2 reasons:
- the intent was to use the default
.bashrcand add the call to restore the configuration files; and - this image is designed to be used in interactive mode and the color in the script output is very helpful.
deploy/docker_home/restore_config.sh line 3 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Is the
/hostsnecessary in/etc/hosts?
It is not necessary but both forms are correct; listing the file makes it clearer (in my mind) which file will be overwritten - helpful for an important system file.
deploy/scripts/check_certs.py line 73 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
If this array is short enough to fit on one line, might as well do the same for two shorter arrays earlier in the script.
That is all formatted by tox -e fmt. If we put it all on one line, the CI Python tests will fail.
deploy/scripts/setup_target.py line 71 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
"the the"
Duplicated for emphasis. (Not really!)
Fixed.
Add a short form output for check_certs.py so that the script is more useful in command pipelines
There was a problem hiding this comment.
Reviewed 3 of 7 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jmgrady)
This PR includes the following enhancements to the
combine_deployimage that can be used to setup The Combine on a NUC:Adds scripts to store/restore setup information to a volume when mounted as
/config; the information saved is:This obviates the need to re-run the
setup_target.pyscript and the Ansible playbook each time the image is run.Adds a
check_cert.pyto print the expiration time of any TLS certificates installed on the target.This change is