feat(#8551): removes python from cht-deploy and some bug fixes #9074

henokgetachew · 2024-04-29T11:38:03Z

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

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on cht-docs
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

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.

scripts/deploy/src/install.js

mrjones-plip

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!

mrjones-plip · 2024-04-29T21:31:29Z

scripts/deploy/cht-deploy

@@ -1,40 +1,43 @@
-#!/bin/bash
+#!/usr/bin/env node


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>

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>

scripts/deploy/src/install.js

nydr

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

scripts/deploy/package.json

scripts/deploy/src/install.js

scripts/deploy/package.json

mrjones-plip

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.

scripts/deploy/troubleshooting/get-all-logs

scripts/deploy/src/install.js

scripts/deploy/cht-deploy

scripts/deploy/src/install.js

Co-authored-by: Daniel <nydr@users.noreply.github.com>

scripts/deploy/src/install.js

henokgetachew · 2024-05-25T10:22:28Z

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.

nydr

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!

scripts/deploy/cht-deploy

Co-authored-by: Daniel <nydr@users.noreply.github.com>

henokgetachew · 2024-05-28T12:40:57Z

@nydr yes, I've actually already published this to npm. https://www.npmjs.com/package/@medic/cht-deploy. It can be run via npx @medic/cht-deploy -f <path-to-your-yaml>

mrjones-plip

Awesome work here! Love the follow through.

A few small things to fix up!

mrjones-plip · 2024-05-28T21:52:51Z

scripts/deploy/cht-deploy

@@ -1,40 +1,43 @@
-#!/bin/bash
+#!/usr/bin/env node


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>

mrjones-plip · 2024-05-28T22:06:07Z

scripts/deploy/troubleshooting/get-all-logs

+  echo
+  echo "Note: Logs may contain Personally Identifiable Information (PII). Handle and store them securely."
+  exit 1
+}qq


calling this now I get an error instead of the usage() output:

➜ deploy git:(cht-deploy-fixes) ✗ ./troubleshooting/get-all-logs ./troubleshooting/get-all-logs: line 55: syntax error: unexpected end of file

Aha! There's two errant qqs that need to be removed:

Suggested change

}qq

}

mrjones-plip · 2024-05-28T22:19:50Z

scripts/deploy/troubleshooting/get-all-logs

+done
+
+# Create a tar.gz archive of the log files
+tar -czf "$NAMESPACE-logs_$TIMESTAMP.tar.gz" -C "$LOG_DIR" .


This syntax causes the log files to be exploded in the current directory when uncompressed, which was a surprise. With a small tweak we can make it be created it in a sub directory:

Suggested change

tar -czf "$NAMESPACE-logs_$TIMESTAMP.tar.gz" -C "$LOG_DIR" .

tar -czf "$NAMESPACE-logs_$TIMESTAMP.tar.gz" "$LOG_DIR"

mrjones-plip · 2024-05-28T22:34:30Z

scripts/deploy/src/install.js

+      child_process.execSync(`helm install ${project_name} ${CHT_CHART_NAME}` + //NoSONAR
+        ` --version ${chart_version} --namespace ${namespace} ${createNamespaceFlag}` + //NoSONAR
+        ` --values ${valuesFile} --set cht_image_tag=${image_tag}`, { stdio: 'inherit' }); //NoSONAR
+      console.log(`Instance at ${values.ingress.host} installed successfully.`);


It's nice to make this a full link:

Suggested change

console.log(`Instance at ${values.ingress.host} installed successfully.`);

console.log(`Instance installed successfully: https://${values.ingress.host}`);

mrjones-plip · 2024-05-28T22:49:44Z

scripts/deploy/src/install.js

+const __filename = fileURLToPath(import.meta.url);
+const __dirname = path.dirname(__filename);
+
+const readFile = function(f) {


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"

dianabarsan

Cool stuff! This makes for a much easier read!

dianabarsan · 2024-05-29T03:36:38Z

scripts/deploy/cht-deploy

-invoke install $@
+#!/usr/bin/env node
+
+import { install } from './src/install.js';


This looks like it isn't linted with our rules.
These are our linting rules: https://github.com/medic/cht-core/blob/master/.eslintrc

dianabarsan · 2024-05-29T03:38:53Z

scripts/deploy/cht-deploy

+            process.exit(1);
+        }
+    } catch (err) {
+        return;


So, if we fail to parse package.json, we'll just go ahead?

dianabarsan · 2024-05-29T03:40:30Z

scripts/deploy/cht-deploy

+    return args;
+}
+
+function validateFileExists(filePath) {


I think validateArguments can also call this function, and only return if the file exists. This way, the main function doesn't even need to know what the arguments are, and becomes cleaner.

dianabarsan · 2024-05-29T03:40:51Z

scripts/deploy/cht-deploy

+
+main().catch((err) => {
+    console.error('An error occurred:', err.message);
+    console.error(JSON.stringify(err));


I think we should process.exit(1) here as well.

dianabarsan · 2024-05-29T03:43:19Z

scripts/deploy/src/certificate.js

+import fetch from 'node-fetch';
+import UserRuntimeError from './error.js';
+
+const obtainCertificateAndKey = async function (values) { //NoSONAR


I think ideally we would never disable sonar for new code.

dianabarsan · 2024-05-29T04:22:25Z

scripts/deploy/tests/helm.test.js

+  const fakeImageTag = 'latest';
+
+  beforeEach(() => {
+    execSyncStub = sinon.stub(child_process, 'execSync');


You don't need to save these stub names. I'm pretty much against passing this variable to tests and then having trouble knowing what it is.
You can always access it as:

sinon.stub(child_process, 'execSync'); child_process.execSync.withArgs(`helm list -n ${fakeNamespace}`).returns(Buffer.from('test-project'));

dianabarsan · 2024-05-29T04:24:23Z

scripts/deploy/tests/helm.test.js

+        ` --install test-project medic/cht-chart-4x` +
+        ` --version 5.0.0 --namespace test-namespace` +
+        ` --values ${fakeValuesFile} --set cht_image_tag=${fakeImageTag}`;
+    expect(execSyncStub.calledWith(expectedCommand)).to.be.true;


I think we also need to assess that execSync was not called with other arguments.

expect(execSyncStub.calledOnceWith(expectedCommand)).to.be.true;

dianabarsan · 2024-05-29T04:26:46Z

scripts/deploy/tests/helm.test.js

+  it('should install a new release when no release exists', () => {
+    // Mock the response to simulate no existing release
+    execSyncStub.withArgs(`helm list -n ${fakeNamespace}`).returns(Buffer.from(''));
+    execSyncStub.returns(Buffer.from('Install successful'));


I think this will overwrite your previous rule of withArgs. You should call them in the reverse order.

child_process.execSync.returns(Buffer.from('Install successful')); child_process.execSync.withArgs(`helm list -n ${fakeNamespace}`).returns(Buffer.from(''));

dianabarsan · 2024-05-29T04:27:20Z

scripts/deploy/tests/helm.test.js

+    const expectedCommand = `helm install test-project medic/cht-chart-4x` + 
+        ` --version 5.0.0 --namespace test-namespace --create-namespace` +
+        ` --values ${fakeValuesFile} --set cht_image_tag=${fakeImageTag}`;
+    expect(execSyncStub.calledWith(expectedCommand)).to.be.true;


expect(child_process.execSync.calledOnceWith(expectedCommand)).to.be.true;

dianabarsan · 2024-05-29T04:30:34Z

scripts/deploy/troubleshooting/get-all-logs

+function usage() {
+  echo "Usage: $0 <namespace> [since]"
+  echo
+  echo "This script saves logs from all pods within the specified namespace into local files and then creates a tar.gz archive of these logs."


I think it's useful to mention here that if a pod crashed, its logs will not be retrieved.
So people can't, unfortunately, use this to debug failing pods.

I think this script can also pessimistically always try to get the logs from the previously failed pod as well as the current one.

dianabarsan · 2024-05-29T04:35:21Z

scripts/deploy/.eslintrc

@@ -0,0 +1,8 @@
+{


I think we should find a way to have these files get linted by our default linting rules: https://github.com/medic/cht-core/blob/master/.eslintrc

henokgetachew requested review from mrjones-plip and nydr April 29, 2024 11:38

github-advanced-security bot found potential problems Apr 29, 2024

View reviewed changes

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

mrjones-plip requested changes Apr 29, 2024

View reviewed changes

dianabarsan requested changes Apr 30, 2024

View reviewed changes

scripts/deploy/src/install.js Outdated Show resolved Hide resolved

henokgetachew mentioned this pull request Apr 30, 2024

CHT Deploy script has error: No such file or directory #9076

Open

github-advanced-security bot found potential problems Apr 30, 2024

View reviewed changes

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

nydr reviewed Apr 30, 2024

View reviewed changes

mrjones-plip requested changes Apr 30, 2024

View reviewed changes

github-advanced-security bot found potential problems May 13, 2024

View reviewed changes

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 15, 2024

View reviewed changes

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

mrjones-plip reviewed May 15, 2024

View reviewed changes

scripts/deploy/cht-deploy Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems May 17, 2024

View reviewed changes

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

jonathanvq added this to Awaiting Triage in Site Reliability Support Dashboard via automation May 21, 2024

jonathanvq moved this from Awaiting Triage to This Week's Priorities in Site Reliability Support Dashboard May 21, 2024

github-advanced-security bot found potential problems May 24, 2024

View reviewed changes

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 24, 2024

View reviewed changes

scripts/deploy/src/install.js Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 25, 2024

View reviewed changes

henokgetachew force-pushed the cht-deploy-fixes branch from 473df1c to ef01030 Compare May 25, 2024 08:19

henokgetachew and others added 11 commits May 25, 2024 11:22

Remove python from cht-deploy

a23311a

Logs archiving functionality

1778603

Show URL in status

9674ae0

Rename command

1443066

Simplify namespace creation

fa4bf18

Replace axios with node-fetch

e8c87a3

Apply suggestions from code review

6069b4b

Co-authored-by: Daniel <nydr@users.noreply.github.com>

Changes based on feedback

657c358

Take out certs and errors

91aeda3

Replace throw with Promise.reject in async func

29c1b7f

Print entire error object for better investigation

3e8a894

henokgetachew added 15 commits May 25, 2024 11:22

Add other commands

70edb5c

Simplify command name for ease

409ec18

Add cht-deploy command back too

ba85477

Check that data.tags has value

8f6531c

Show error detail

117ee87

Bug fix - don't try to create ns if it exists

41df07b

Remove let

775a175

Bug fix - trim isnt removing trailing slash

4cb5e50

Add .gitignore

762b6d9

Add since param based on review

b82289b

Add PII warning to logs download

d21c08c

Refactor goes beyond the scope of this PR

c150052

Move NoSONAR

cada40a

eslint conf

1ad1f1f

Linting, adding tests to run with ci

1537096

henokgetachew force-pushed the cht-deploy-fixes branch from ef01030 to 1537096 Compare May 25, 2024 08:28

NoSONAR

c5950f2

github-advanced-security bot found potential problems May 25, 2024

View reviewed changes

scripts/deploy/src/install.js Dismissed Show dismissed Hide dismissed

scripts/deploy/src/install.js Dismissed Show dismissed Hide dismissed

henokgetachew requested review from nydr, dianabarsan and mrjones-plip May 25, 2024 10:46

nydr approved these changes May 28, 2024

View reviewed changes

scripts/deploy/cht-deploy Outdated Show resolved Hide resolved

Update scripts/deploy/cht-deploy

4760b5d

Co-authored-by: Daniel <nydr@users.noreply.github.com>

mrjones-plip requested changes May 28, 2024

View reviewed changes

dianabarsan requested changes May 29, 2024

View reviewed changes

dianabarsan reviewed May 29, 2024

View reviewed changes

henokgetachew moved this from This Week's Priorities to In Progress in Site Reliability Support Dashboard May 29, 2024

mrjones-plip mentioned this pull request May 31, 2024

CHT deploy script error: upgrade-service image referenced incorrectly #9077

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#8551): removes python from cht-deploy and some bug fixes #9074

feat(#8551): removes python from cht-deploy and some bug fixes #9074

henokgetachew commented Apr 29, 2024 •

edited by github-actions bot

mrjones-plip left a comment

mrjones-plip Apr 29, 2024

henokgetachew May 15, 2024

mrjones-plip May 28, 2024 •

edited

nydr left a comment •

edited

mrjones-plip left a comment

henokgetachew commented May 25, 2024 •

edited

nydr left a comment

henokgetachew commented May 28, 2024

mrjones-plip left a comment

mrjones-plip May 28, 2024 •

edited

mrjones-plip May 28, 2024

mrjones-plip May 28, 2024

mrjones-plip May 28, 2024

mrjones-plip May 28, 2024

dianabarsan left a comment

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024

dianabarsan May 29, 2024 •

edited

	tar -czf "$NAMESPACE-logs_$TIMESTAMP.tar.gz" -C "$LOG_DIR" .
	tar -czf "$NAMESPACE-logs_$TIMESTAMP.tar.gz" "$LOG_DIR"

	console.log(`Instance at ${values.ingress.host} installed successfully.`);
	console.log(`Instance installed successfully: https://${values.ingress.host}`);

feat(#8551): removes python from cht-deploy and some bug fixes #9074

Are you sure you want to change the base?

feat(#8551): removes python from cht-deploy and some bug fixes #9074

Conversation

henokgetachew commented Apr 29, 2024 • edited by github-actions bot

Description

Code review checklist

Compose URLs

License

mrjones-plip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrjones-plip May 28, 2024 • edited

Choose a reason for hiding this comment

nydr left a comment • edited

Choose a reason for hiding this comment

mrjones-plip left a comment

Choose a reason for hiding this comment

henokgetachew commented May 25, 2024 • edited

nydr left a comment

Choose a reason for hiding this comment

henokgetachew commented May 28, 2024

mrjones-plip left a comment

Choose a reason for hiding this comment

mrjones-plip May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianabarsan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianabarsan May 29, 2024 • edited

Choose a reason for hiding this comment

henokgetachew commented Apr 29, 2024 •

edited by github-actions bot

mrjones-plip May 28, 2024 •

edited

nydr left a comment •

edited

henokgetachew commented May 25, 2024 •

edited

mrjones-plip May 28, 2024 •

edited

dianabarsan May 29, 2024 •

edited