-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
feat(#8551): removes python from cht-deploy and some bug fixes #9074
Conversation
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.
Awesome work here! Nice to see a migration to node for user/developer facing scripts. As well, I saw you closed some other issues with this PR like #8604, #8605 and #8608 - sweet!
I found two errors - one I'm requesting a change for and the other is in master
, but persists in this branch. Let's make the change per my request and then wait til the other issue fixed and we'll dive back into this PR!
@@ -1,40 +1,43 @@ | |||
#!/bin/bash | |||
#!/usr/bin/env node |
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.
before the script does anything else, I think we need to check for the node
version. The reason is bash
is very universal, but who knows what version of node
folks have. My desktop defaults to 12.0.0
which pretty ensures it'll break everything, for example. Per this post, looks like we can easily get the SemVer of node back to old versions.
here's my shell session when debugging this:
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy
/home/mrjones/Documents/MedicMobile/cht-core/node_modules/axios/index.js:1
import axios from './lib/axios.js';
^^^^^
SyntaxError: Unexpected identifier
at Module._compile (internal/modules/cjs/loader.js:703:23)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
at Module.load (internal/modules/cjs/loader.js:628:32)
at Function.Module._load (internal/modules/cjs/loader.js:555:12)
at Module.require (internal/modules/cjs/loader.js:666:19)
at require (internal/modules/cjs/helpers.js:16:16)
at Object.<anonymous> (/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/src/install.js:3:15)
at Module._compile (internal/modules/cjs/loader.js:759:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
at Module.load (internal/modules/cjs/loader.js:628:32)
➜ deploy git:(cht-deploy-fixes) ✗ node
Welcome to Node.js v12.0.0.
Type ".help" for more information.
> process.versions.node.split('.').map(Number)
[ 12, 0, 0 ]
>
➜ deploy git:(cht-deploy-fixes) ✗ nvm use 18
Now using node v18.16.0 (npm v9.5.1)
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy
No values file provided. Please specify a values file using -f <file>
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.
Added.
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.
I'm not sure if this was fixed and then a regression was caused, but this doesn't work for me, all be it with a different error than when I first ran it:
➜ deploy git:(cht-deploy-fixes) ✗ nvm use 12.0.0
Now using node v12.0.0 (npm v6.9.0)
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy
/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/cht-deploy:3
import { install } from './src/install.js';
^
SyntaxError: Unexpected token {
at Module._compile (internal/modules/cjs/loader.js:703:23)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
at Module.load (internal/modules/cjs/loader.js:628:32)
at Function.Module._load (internal/modules/cjs/loader.js:555:12)
at Function.Module.runMain (internal/modules/cjs/loader.js:826:10)
at internal/main/run_main_module.js:17:11
➜ deploy git:(cht-deploy-fixes) ✗ nvm use 20
Now using node v20.11.0 (npm v10.2.4)
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy
No values file provided. Please specify a values file using -f <file>
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.
Great initiative! I'm happy to see significant progress in replacing the old script.
I would love to see a test, even if it's just trying to import the module or testing the easiest to test function as it can catch some syntax errors and make it easier to add more tests later. Since the old script didn't have any tests I don't consider this a blocker, even in the current state I consider it an significant improvement.
I think some jsdoc style comments on most functions would be great
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.
Looking better - thanks for iterating!
Confirmed that #9076 is fixed with the latest changes. As well, sad path and happy paths are well covered!
Requesting some security and changes to new save all logs script.
ef01030
to
1537096
Compare
Comments addressed. A few tests added and build is passing. Also integrated with CI. This puts us in a better spot that we can iterate on. |
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.
Nice job! One small comment for displaying of errors
I'd like to see this package published to a registry or be in it's own repo so it can be called directly using npx or similar, but not a requirement for this PR, still a great step forward!
@nydr yes, I've actually already published this to |
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.
Awesome work here! Love the follow through.
A few small things to fix up!
@@ -1,40 +1,43 @@ | |||
#!/bin/bash | |||
#!/usr/bin/env node |
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.
I'm not sure if this was fixed and then a regression was caused, but this doesn't work for me, all be it with a different error than when I first ran it:
➜ deploy git:(cht-deploy-fixes) ✗ nvm use 12.0.0
Now using node v12.0.0 (npm v6.9.0)
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy
/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/cht-deploy:3
import { install } from './src/install.js';
^
SyntaxError: Unexpected token {
at Module._compile (internal/modules/cjs/loader.js:703:23)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
at Module.load (internal/modules/cjs/loader.js:628:32)
at Function.Module._load (internal/modules/cjs/loader.js:555:12)
at Function.Module.runMain (internal/modules/cjs/loader.js:826:10)
at internal/main/run_main_module.js:17:11
➜ deploy git:(cht-deploy-fixes) ✗ nvm use 20
Now using node v20.11.0 (npm v10.2.4)
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy
No values file provided. Please specify a values file using -f <file>
const __filename = fileURLToPath(import.meta.url); | ||
const __dirname = path.dirname(__filename); | ||
|
||
const readFile = function(f) { |
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.
I'm not sure if it's worth defending against, but I passed in a project_name and namespace of this:
project_name: mrjones-delete
namespace: "mrjones-delete"
...
host: "mrjones-delete.dev.medicmobile.org"
And it created it OK apparently:
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy -f ~/Documents/MedicMobile/medic-infrastructure/terraform/aws/dev/cht-projects/mrjones-dev-cht-deploy-values.yaml
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "medic" chart repository
Update Complete. ⎈Happy Helming!⎈
Release does not exist. Performing install.
NAME: mrjones-delete
LAST DEPLOYED: Tue May 28 15:28:12 2024
NAMESPACE: mrjones-delete
STATUS: deployed
REVISION: 1
TEST SUITE: None
Instance at mrjones-delete.dev.medicmobile.org installed successfully.
But I can't get to it at https://mrjones-delete.dev.medicmobile.org/
- it just 404s. I think this is something endemic to our EKS setup and most other folks won't benefit from improvements here - but wanted to mention it in case it was helpful!
Switching to sane values it worked as expected \o/
project_name: mrjones-dev
namespace: "mrjones-dev"
...
host: "mrjones.dev.medicmobile.org"
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.
Cool stuff! This makes for a much easier read!
invoke install $@ | ||
#!/usr/bin/env node | ||
|
||
import { install } from './src/install.js'; |
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.
This looks like it isn't linted with our rules.
These are our linting rules: https://github.com/medic/cht-core/blob/master/.eslintrc
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.
This file was skipped. Fixed.
The whole thing is linted by our linting rules with overrides configured here: https://github.com/medic/cht-core/blob/cht-deploy-fixes/scripts/deploy/.eslintrc
scripts/deploy/src/certificate.js
Outdated
import fetch from 'node-fetch'; | ||
import UserRuntimeError from './error.js'; | ||
|
||
const obtainCertificateAndKey = async function (values) { //NoSONAR |
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.
I think ideally we would never disable sonar for new code.
Co-authored-by: Daniel <nydr@users.noreply.github.com>
Co-authored-by: mrjones <8253488+mrjones-plip@users.noreply.github.com>
Co-authored-by: mrjones <8253488+mrjones-plip@users.noreply.github.com>
ffd255e
to
363f50c
Compare
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.
Nice work on this! It was a really big effort and we're much better off because of the features added, bugs fixed and totally rewriting it in node.
I'm approving PR after testing the happy paths and not seeing anything amiss. I note some room for improvements below, but let's merge this!
defending against old node
This works but only if you're running newer node. Here's what I see running node 12 vs 18:
➜ deploy git:(cht-deploy-fixes) node --version
v12.22.12
➜ deploy git:(cht-deploy-fixes) ./cht-deploy
internal/process/esm_loader.js:74
internalBinding('errors').triggerUncaughtException(
^
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for /var/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/cht-deploy
at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:65:15)
at Loader.getFormat (internal/modules/esm/loader.js:116:42)
at Loader.getModuleJob (internal/modules/esm/loader.js:247:31)
at async Loader.import (internal/modules/esm/loader.js:181:17)
at async Object.loadESM (internal/process/esm_loader.js:68:5) {
code: 'ERR_UNKNOWN_FILE_EXTENSION'
}
➜ deploy git:(cht-deploy-fixes) ✗ nvm use 18
Now using node v18.20.4 (npm v10.7.0)
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy
Invalid Node.js version. Required: >=20.11.0. Current: v18.20.4
not being logged in
This could be smoother. I don't know if there's an explicit way to check if the user is logged in and just show "please run eks-aws-mfa-login
before proceeding", but this is a bit messy:
➜ deploy git:(cht-deploy-fixes) ✗ ./cht-deploy -f mrjones-dev.yml
Helm repo medic not found, adding..
"medic" has been added to your repositories
E0816 15:08:49.499059 101343 memcache.go:265] "Unhandled Error" err="couldn't get current server API group list: the server has asked for the client to provide credentials"
E0816 15:08:50.680340 101343 memcache.go:265] "Unhandled Error" err="couldn't get current server API group list: the server has asked for the client to provide credentials"
E0816 15:08:51.848484 101343 memcache.go:265] "Unhandled Error" err="couldn't get current server API group list: the server has asked for the client to provide credentials"
E0816 15:08:53.082776 101343 memcache.go:265] "Unhandled Error" err="couldn't get current server API group list: the server has asked for the client to provide credentials"
E0816 15:08:54.262058 101343 memcache.go:265] "Unhandled Error" err="couldn't get current server API group list: the server has asked for the client to provide credentials"
error: You must be logged in to the server (the server has asked for the client to provide credentials)
Error: Kubernetes cluster unreachable: the server has asked for the client to provide credentials
Command failed: helm list -n mrjones-dev
Error: Kubernetes cluster unreachable: the server has asked for the client to provide credentials
Error: Command failed: helm list -n mrjones-dev
Error: Kubernetes cluster unreachable: the server has asked for the client to provide credentials
at genericNodeError (node:internal/errors:984:15)
at wrappedFn (node:internal/errors:538:14)
at checkExecSyncError (node:child_process:890:11)
at Object.execSync (node:child_process:962:15)
at helmInstallOrUpdate (file:///var/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/src/install.js:88:41)
at install (file:///var/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/src/install.js:165:3)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async runInstallScript (file:///var/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/cht-deploy:55:5)
at async main (file:///var/home/mrjones/Documents/MedicMobile/cht-core/scripts/deploy/cht-deploy:66:3)
invalid password
This is hard to defend against, but it seems not good to accept a values.yml
with a value of password: BankLogin!3
. While I know we have an extensive section on how to use a valid password in our docs, the system just left with a 503
, and cht-api
pod not running and this error in the logs:
2024-08-16T22:12:41.512 INFO: Configuration loaded successfully
2024-08-16T22:12:41.513 INFO: Running db migrations…
2024-08-16T22:12:41.537 INFO: Running migration associate-records-with-people...
2024-08-16T22:12:41.545 ERROR: Migration associate-records-with-people failed
2024-08-16T22:12:41.546 ERROR: Fatal error initialising API
2024-08-16T22:12:41.549 ERROR: StatusCodeError: 401 - {"error":"unauthorized","reason":"Name or password is incorrect."}
at new StatusCodeError (/service/api/node_modules/request-promise-core/lib/errors.js:32:15)
at Request.plumbing.callback (/service/api/node_modules/request-promise-core/lib/plumbing.js:104:33)
at Request.RP$callback [as _callback] (/service/api/node_modules/request-promise-core/lib/plumbing.js:46:31)
at Request.self.callback (/service/api/node_modules/request/request.js:185:22)
at Request.emit (node:events:513:28)
at Request.<anonymous> (/service/api/node_modules/request/request.js:1154:10)
at Request.emit (node:events:513:28)
at IncomingMessage.<anonymous> (/service/api/node_modules/request/request.js:1076:12)
at Object.onceWrapper (node:events:627:28)
at IncomingMessage.emit (node:events:525:35) {
name: 'StatusCodeError',
statusCode: 401,
message: '401 - {"error":"unauthorized","reason":"Name or password is incorrect."}',
error: { error: 'unauthorized', reason: 'Name or password is incorrect.' },
[stack]: 'StatusCodeError: 401 - {"error":"unauthorized","reason":"Name or password is incorrect."}\n' +
' at new StatusCodeError (/service/api/node_modules/request-promise-core/lib/errors.js:32:15)\n' +
' at Request.plumbing.callback (/service/api/node_modules/request-promise-core/lib/plumbing.js:104:33)\n' +
' at Request.RP$callback [as _callback] (/service/api/node_modules/request-promise-core/lib/plumbing.js:46:31)\n' +
' at Request.self.callback (/service/api/node_modules/request/request.js:185:22)\n' +
' at Request.emit (node:events:513:28)\n' +
' at Request.<anonymous> (/service/api/node_modules/request/request.js:1154:10)\n' +
' at Request.emit (node:events:513:28)\n' +
' at IncomingMessage.<anonymous> (/service/api/node_modules/request/request.js:1076:12)\n' +
' at Object.onceWrapper (node:events:627:28)\n' +
' at IncomingMessage.emit (node:events:525:35)'
}
Description
Handles the following tickets:
#8551 Remove python - establishing feature parity. No major refactoring.
#8604 Catch Missing Values
#8605 Show URL when done execution
#8608 Add a get-all-logs troubleshooting command
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.