-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Create script to install The Combine on a standalone system #2976
Conversation
Also: * In `src/index.tsx` replace `import { render } from "react-dom";` with `import { createRoot } from "react-dom/client"` (per https://react.dev/blog/2022/03/08/react-18-upgrade-guide#updates-to-client-rendering-apis) * Update `@material-table/core` from 6.2 to 6.3 (not compatible with react 17) * Remove dependency `@mui/styles` (not compatible with react 18); replace with either `import { styled } from "@mui/system";` or direct `sx=` * Remove extra space from between audio components * Update `@microsoft/signalr` from 6 to 8 * Add dev dependency:`@babel/plugin-proposal-private-property-in-object` to deal with legacy warning * Migrate `@testing-library/react-hooks` usage to the corresponding parts in `@testing-library/react` * Remove unnecessary/problematic `await act(...` instances from `@testing-library/react tests` * Use `as any` to handle `TextFieldWithFont` type issues * Replace deprecated `tobeCalled` with `toHaveBeenCalled` and `tobeCalledTimes` with `toHaveBeenCalledTimes` * Remove extra space from between audio components
* Bump @adobe/css-tools from 4.3.1 to 4.3.2 * Update Python dependencies * Bump Microsoft.AspNetCore.Authentication.JwtBearer in /Backend * Bump MailKit from 4.2.0 to 4.3.0 in /Backend * Bump node from 18.18.0-bookworm-slim to 18.18.2-bookworm-slim * Bump @mui/icons-material from 5.14.18 to 5.14.19 * Bump react-i18next from 13.4.1 to 13.5.0 * Bump license-checker-rseidelsohn from 4.2.10 to 4.2.11 * Bump @types/react-test-renderer from 18.0.6 to 18.0.7 * Bump Microsoft.NET.Test.Sdk from 17.7.2 to 17.8.0 in /Backend.Tests * Bump NUnit from 3.13.3 to 4.0.0 in /Backend.Tests * Bump step-security/harden-runner from 2.6.0 to 2.6.1 * Bump github/codeql-action from 2.22.5 to 2.22.8 * Bump docker/build-push-action from 5.0.0 to 5.1.0 * Bump mongo from 7.0.2-jammy to 7.0.4-jammy in /database * Update license reports --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 18 files at r2, 6 of 23 files at r3, all commit messages.
Reviewable status: 8 of 54 files reviewed, 5 unresolved discussions (waiting on @jmgrady)
README.md
line 130 at r3 (raw file):
11. [dotnet-project-licenses](https://github.com/tomchavakis/nuget-license) `dotnet tool update --global dotnet-project-licenses` 12. Tools for generating the self installer [Linux Only]:
(Linux only)
instead of [Linux Only]
to match elsewhere.
README.md
line 134 at r3 (raw file):
- [makeself](https://makeself.io/) - a tool to make self-extracting archives in Unix - [pandoc](https://pandoc.org/installing.html#linux) - a tool to convert Markdown documents to PDF. In addition to `pandoc`, the `weasyprint` PDF engine is required. These may be installed on Debian-based distributions by
WeasyPrint should perhaps be its own bullet.
README.md
line 518 at r3 (raw file):
Generate Installer Script for The Combine (Linux Only)
(Linux only)
instead of (_Linux Only_)
to match elsewhere.
Also, this item needs to be added to the contents at top.
README.md
line 527 at r3 (raw file):
where
combine-release-number
is the Combine release to be installed, e.g.v1.2.0
.
How about <combine-release-number>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 47 files at r1, 3 of 18 files at r2, 5 of 23 files at r3.
Reviewable status: 22 of 54 files reviewed, 9 unresolved discussions (waiting on @jmgrady)
deploy/ansible/roles/wifi_ap/tasks/main.yml
line 12 at r3 (raw file):
# file: test.yml # tags: # - test
Is the commented out test meant to persist?
deploy/scripts/setup_files/combine_config.yaml
line 24 at r3 (raw file):
profile: desktop env_vars_required: false override:
Missing the # override values for 'thecombine' chart
comment that the rest have.
installer/README.md
line 5 at r3 (raw file):
This README describes how to install _The Combine_ Rapid Word Collection tool on a laptop or desktop PC. ## Contents
The top two items in the Contents list are ###
below, and the bottom two are ##
below.
deploy/ansible/roles/container_engine/tasks/main.yml
line 3 at r3 (raw file):
--- ############################################################## # Role: docker_install
Update comment to fit new name/role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 47 files at r1.
Reviewable status: 26 of 54 files reviewed, 13 unresolved discussions (waiting on @jmgrady)
a discussion (no related file):
There's a deploy/ansible/roles/package_install
role that's not referenced anywhere. Is that still needed?
deploy/ansible/playbook_desktop_setup.yaml
line 9 at r3 (raw file):
# as Docker containers managed by a Kubernetes cluster. # ##############################################################
Update comment for this filename.
deploy/ansible/roles/wifi_ap/defaults/main.yml
line 5 at r3 (raw file):
ap_passphrase: "Set a new passphrase in your config file." ap_gateway: "10.10.10.1" ap_domain: example.com
I don't see any use or replacement instructions for "example.com".
deploy/ansible/playbook_nuc_setup.yaml
line 9 at r3 (raw file):
# as Docker containers managed by a Kubernetes cluster. # ##############################################################
Update comment for this filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @Github-advanced-security[bot] from a discussion.
Reviewable status: 25 of 57 files reviewed, 4 unresolved discussions (waiting on @imnasnainaec and @jmgrady)
a discussion (no related file):
Previously, imnasnainaec (D. Ror.) wrote…
There's a
deploy/ansible/roles/package_install
role that's not referenced anywhere. Is that still needed?
No it is not. Removed.
README.md
line 130 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
(Linux only)
instead of[Linux Only]
to match elsewhere.
Done
README.md
line 134 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
WeasyPrint should perhaps be its own bullet.
Done
README.md
line 518 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
(Linux only)
instead of(_Linux Only_)
to match elsewhere.Also, this item needs to be added to the contents at top.
Done
README.md
line 527 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
How about
<combine-release-number>
?
I thought about that but < > are input/output redirection operator in bash
. I also thought about[combine-release-number]
but that makes it look like it's optional.
deploy/ansible/playbook_desktop_setup.yaml
line 9 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Update comment for this filename.
Done
deploy/ansible/roles/wifi_ap/defaults/main.yml
line 5 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I don't see any use or replacement instructions for "example.com".
Changed the default value to thecombine.app
.
deploy/ansible/roles/wifi_ap/tasks/main.yml
line 12 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Is the commented out test meant to persist?
In general, it has been problematic and does not really work in the desktop environment since a reboot was often required before the test is run. It has been restored but there is a condition added so that a host_vars/group_vars configuration can disable it.
deploy/scripts/setup_combine.py
line 148 at r2 (raw file):
Previously, github-advanced-security[bot] wrote…
Clear-text logging of sensitive information
This expression logs sensitive data (secret) as clear text.
This expression logs sensitive data (secret) as clear text.
Done.
deploy/scripts/setup_files/combine_config.yaml
line 24 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Missing the
# override values for 'thecombine' chart
comment that the rest have.
Done
deploy/ansible/playbook_nuc_setup.yaml
line 9 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Update comment for this filename.
Done
deploy/ansible/roles/container_engine/tasks/main.yml
line 3 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Update comment to fit new name/role.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 47 files at r1, 2 of 18 files at r2, 3 of 23 files at r3, 8 of 12 files at r4, all commit messages.
Reviewable status: 43 of 57 files reviewed, 7 unresolved discussions (waiting on @jmgrady)
README.md
line 130 at r3 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Done
[]
-> ()
was my primary intent.
README.md
line 518 at r3 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Done
Removing the _
s was my primary intent.
deploy/ansible/playbook_desktop_setup.yaml
line 9 at r3 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Done
The file is _setup.yaml
but the comment has _install.yaml
.
deploy/ansible/group_vars/nuc/main.yml
line 58 at r4 (raw file):
ap_passphrase: "Combine2020" ap_gateway: "10.10.10.1" ap_hostname: "{{ansible_hostname}}"
Should test_wifi
be added here too?
deploy/ansible/roles/support_tools/files/display-eth-addr.sh
line 18 at r4 (raw file):
# set red background echo -en \\xfe\\xd0\\xff\\x1f\\x1f > ${TTY_DEV} # 1234567890123456
What's the 1234567890123456
comment for?
deploy/ansible/roles/support_tools/tasks/main.yml
line 20 at r4 (raw file):
with_items: - display-eth.service - display-eth.timer
Is there any redundancy having
with_items:
- display-eth.service
- display-eth.timer
both here and in deploy/ansible/roles/support_tools/handlers/main.yml
?
deploy/ansible/roles/support_tools/templates/display-eth.timer.j2
line 2 at r4 (raw file):
[Unit] Description=Display Ethernet Address on support tool display
I don't understand how this description fits the "timer" name and meat of this file?
deploy/ansible/roles/wifi_ap/tasks/install.yml
line 67 at r4 (raw file):
lineinfile: path: /etc/hosts regexp: ^{{ ap_gateway }}
I guess the .
s in the ap_gateway
string don't need to be escaped.
deploy/scripts/setup_combine.py
line 168 at r4 (raw file):
def get_target(config: Dict[str, Any]) -> str: """List available targets and get selection from the user.""" print("Available targets:")
Several print
statements remain in this file.
deploy/ansible/playbook_nuc_setup.yaml
line 9 at r3 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Done
The file is _setup.yaml
but the comment has _install.yaml
.
deploy/ansible/roles/support_tools/files/combinectl.sh
line 23 at r4 (raw file):
updated install package. wifi [wifi-passphrase]: If no parameters are provieded, display the wifi
typo: "provieded"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 43 of 57 files reviewed, 3 unresolved discussions (waiting on @imnasnainaec and @jmgrady)
README.md
line 130 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
[]
->()
was my primary intent.
Done.
README.md
line 518 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Removing the
_
s was my primary intent.
Done.
I prefer the emphasis there, though. If you agree, at a future date we can add the emphasis there and for the cases that are Windows only.
deploy/ansible/playbook_desktop_setup.yaml
line 9 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
The file is
_setup.yaml
but the comment has_install.yaml
.
Done.
deploy/ansible/group_vars/nuc/main.yml
line 58 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Should
test_wifi
be added here too?
No. The default is 'true'. Only deploy/ansible/host_vars/localhost/main.yml
needs to set it.
deploy/ansible/roles/support_tools/files/display-eth-addr.sh
line 18 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
What's the
1234567890123456
comment for?
Done.
/dev/ttyACM0
is a 16 character text display (https://www.adafruit.com/product/782).
The comment was to help me center the "NO ETHERNET" and "CONNECTED" text.
It has been removed.
deploy/ansible/roles/support_tools/tasks/main.yml
line 20 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Is there any redundancy having
with_items: - display-eth.service - display-eth.timer
both here and in
deploy/ansible/roles/support_tools/handlers/main.yml
?
No. In tasks/main.yml
it is creating the service files; in handlers/main.yml
it is starting the services if the service files changed. The handler will run at the end of the playbook - not at the end of the task.
deploy/ansible/roles/support_tools/templates/display-eth.timer.j2
line 2 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I don't understand how this description fits the "timer" name and meat of this file?
Done.
Yes, that was a poor description.
deploy/ansible/roles/wifi_ap/tasks/install.yml
line 67 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I guess the
.
s in theap_gateway
string don't need to be escaped.
Done.
Added escapes for the .
characters.
deploy/scripts/setup_combine.py
line 168 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Several
Yes. setup_combine.py
is interactive if there are multiple Kubernetes clusters configured or multiple AWS credentials configured. This helps to prevent installing on the wrong cluster with the wrong credentials - something I found easy to do. I will probably be the only one who sees this.
deploy/ansible/playbook_nuc_setup.yaml
line 9 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
The file is
_setup.yaml
but the comment has_install.yaml
.
Done.
deploy/ansible/roles/support_tools/files/combinectl.sh
line 23 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
typo: "provieded"
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 47 files at r1, 2 of 23 files at r3, 4 of 7 files at r5, all commit messages.
Reviewable status: 50 of 57 files reviewed, 2 unresolved discussions (waiting on @jmgrady)
deploy/ansible/group_vars/nuc/main.yml
line 58 at r4 (raw file):
Previously, jmgrady (Jim Grady) wrote…
No. The default is 'true'. Only
deploy/ansible/host_vars/localhost/main.yml
needs to set it.
Ah, so test_wifi: true
in deploy\ansible\roles\wifi_ap\defaults\main.yml
is unnecessary.
deploy/ansible/roles/support_tools/files/display-eth-addr.sh
line 6 at r5 (raw file):
if [ -e "$TTY_DEV" ] ; then ETH_ADD_STR=`ip -4 -br address | grep "^en"`
In Windows 11 WSL, none of the lines I get from ip -4 -br address
start with en
, just lo
, eth0
, flannel.1
, and cni0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 50 of 57 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec and @jmgrady)
deploy/ansible/group_vars/nuc/main.yml
line 58 at r4 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Ah, so
test_wifi: true
indeploy\ansible\roles\wifi_ap\defaults\main.yml
is unnecessary.
No. That is what defines the default value to be true
. If that line was not there, you would get an error saying that test_wifi
is undefined when you tried to install on a NUC.
For more info, see:
- https://docs.ansible.com/ansible/2.9/user_guide/playbooks_reuse_roles.html#role-default-variables
- https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_variables.html#where-to-set-variables
deploy/ansible/roles/support_tools/files/display-eth-addr.sh
line 6 at r5 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
In Windows 11 WSL, none of the lines I get from
ip -4 -br address
start withen
, justlo
,eth0
,flannel.1
, andcni0
.
This looks like an issue with WSL. It says it allows systemd
but is not using the systemd` network naming conventions - see https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/
The eth0
, wlan0
etc. names were used previously but were phased out to have more predictable (but less meaningful to us mere humans) names.
The display-eth-addr.sh
is only meant to be run to help connect to headless systems. I will turn it off for the localhost install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 47 files at r1, 1 of 23 files at r3, 2 of 2 files at r6, all commit messages.
Reviewable status: 53 of 57 files reviewed, 4 unresolved discussions (waiting on @jmgrady)
deploy/ansible/roles/support_tools/files/display-eth-addr.sh
line 15 at r6 (raw file):
# Move cursor to origin echo -en \\xfe\\x48 > ${TTY_DEV} if [[ $ETH_IP == "" && $ETH_STATUS == "" ]] ; then
Since the case of [[ $ETH_IP == "" && $ETH_STATUS != "" ]]
doesn't seem to be a concern, why not just [[ $ETH_IP == "" ]]
here?
deploy/scripts/uninstall-combine
line 4 at r6 (raw file):
delete-files () { for file in "$@" ; do
Do we want any kind of safety check on file
in this function in case (e.g.) somebody accidentally replaces delete-files /etc/create_ap
below with delete-files / etc/create_ap
?
installer/README.md
line 26 at r6 (raw file):
## Install _The Combine_ 1. Plug in the wired ethernet connection to the Internet.
It would be good to know whether it matters if the wifi is on or off.
deploy/ansible/roles/support_tools/files/combinectl.sh
line 49 at r6 (raw file):
# Restart a WiFi connection that was saved previously restore-wifi-connection () { if [ -f "${COMBINE_CONFIG}/wifi_connection.txt" ] ; then
Since ${COMBINE_CONFIG}/wifi_connection.txt
, is thrice used, it could be given to a variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 53 of 57 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec and @jmgrady)
deploy/ansible/roles/support_tools/files/display-eth-addr.sh
line 15 at r6 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Since the case of
[[ $ETH_IP == "" && $ETH_STATUS != "" ]]
doesn't seem to be a concern, why not just[[ $ETH_IP == "" ]]
here?
Done.
When the ethernet cable is not connected, ip -4 -br address
does not list it. Therefore the return string, after grep removes all lines that don't match ^en
, is empty. Changed the structure to say NO ETHERNET when the device is not in the output.
Also updated to script to be (hopefully) more readable.
deploy/scripts/uninstall-combine
line 4 at r6 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Do we want any kind of safety check on
file
in this function in case (e.g.) somebody accidentally replacesdelete-files /etc/create_ap
below withdelete-files / etc/create_ap
?
I don't think so. I don't think scripts should try to second guess the author.
installer/README.md
line 26 at r6 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
It would be good to know whether it matters if the wifi is on or off.
Updated README.md
deploy/ansible/roles/support_tools/files/combinectl.sh
line 49 at r6 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Since
${COMBINE_CONFIG}/wifi_connection.txt
, is thrice used, it could be given to a variable.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r7, all commit messages.
Reviewable status: 55 of 57 files reviewed, 2 unresolved discussions (waiting on @jmgrady)
deploy/ansible/roles/support_tools/files/combinectl.sh
line 167 at r7 (raw file):
mkdir -p "${COMBINE_CONFIG}" # Print usage is command is missing
Type "is command" -> "if command"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 55 of 57 files reviewed, 1 unresolved discussion
deploy/ansible/roles/support_tools/files/combinectl.sh
line 167 at r7 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Type "is command" -> "if command"
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 23 files at r3, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jmgrady)
The installer and the installation README.PDF can be downloaded from
When the installer script is run, it will
playbook_desktop_setup.yml
to install the prerequisite software, for example,create_ap
to create a WiFi access pointk3s
Kubernetes enginekubectl
helm
combinectl
to manage The Combine post-installationcert-manager
andnginx-ingress-controller
helm chartsIf the script does not finish, the next run will pick up where it left off.
Closes #2784, closes #2531
This change is