action to: update app_settings, set medic-credentials #1

kitsao · 2022-03-24T21:28:17Z

Updates app_settings.json
Updates medic-credentials in CouchDB

derickl

It's shaping up well @kitsao. Great progress.

I've left some feedback inline. Do include some static analysis checking e.g eslint

derickl · 2022-03-25T14:21:15Z

dynamic-rapidpro-workspace-settings/index.js

+const settingsFile = 'app_settings.json';
+const flowsFile = 'flows.js';
+
+run(githubWorkspacePath, core, fs);


run is an async function and as such should be awaited.

For this scenario, you could use an IIFE

(async() => { const githubWorkspacePath = process.env['GITHUB_WORKSPACE']; const settingsFile = 'app_settings.json'; const flowsFile = 'flows.js'; run(githubWorkspacePath, core, fs); )();

derickl · 2022-03-25T14:24:20Z

dynamic-rapidpro-workspace-settings/package.json

+    "test": "mocha test/*.spec.js --timeout 10000 --slow 500"
+  },
+  "author": "Medic Inc.",
+  "license": "ISC",


Medic code is released under AGPL (AGPL-3.0-only)

Most repos use ISC. Updated to AGPL-3.0-only.

derickl · 2022-03-25T14:24:40Z

dynamic-rapidpro-workspace-settings/package.json

+    "build": "ncc build index.js --license licenses.txt",
+    "test": "mocha test/*.spec.js --timeout 10000 --slow 500"
+  },
+  "author": "Medic Inc.",


Unsure if we use this in our package.json files

derickl · 2022-03-25T14:25:03Z

dynamic-rapidpro-workspace-settings/package.json

+    "axios": "^0.21.4",
+    "chai": "^4.3.6",
+    "fs": "^0.0.1-security",
+    "mocha": "^9.2.1",


mocha and chai should be dev dependencies

derickl · 2022-03-25T14:26:45Z

dynamic-rapidpro-workspace-settings/test/env.js

@@ -0,0 +1,26 @@
+
+const { Factory } = require('rosie');
+const { faker } = require('@faker-js/faker');


Nit: I don't think rosie should be a hard dependency since we just need to cook up a couple of field values

Moved to devDependencies.

derickl · 2022-03-25T14:27:50Z

dynamic-rapidpro-workspace-settings/test/index.spec.js

+describe(`rapidpro action test suite`, () => {
+  const mockedAxiosResponse = {
+    data: {},
+    status: 200,


How would the code behave with a non-2xx response?

Added an extra test.

derickl · 2022-03-25T14:29:36Z

dynamic-rapidpro-workspace-settings/test/index.spec.js

+
+  it(`should fail if no argument is passed to get required secrets`, async () => {
+    expect( function () {
+      utils.getInputs();


What happens if we pass sone random parameters?

e.g {}, null, undefined

Expected to fail, throw Error.

derickl · 2022-03-25T14:32:53Z

dynamic-rapidpro-workspace-settings/test/index.spec.js

+    try {
+      await utils.getReplacedContent(settings);
+    } catch (err) {
+      expect(err).to.include(new Error());


I would expect to use to include with containers e.g arrays etc.

Consider using the expect(...).to.throw(Error Class) pattern

This works for sync functions only, because async functions return a promise. Try catch easily handles asyc functions to catch and throw errors.

derickl · 2022-03-31T06:42:55Z

Potential usecase for this as well: medic/cht-conf#348

derickl · 2022-03-29T19:52:20Z

dynamic-rapidpro-workspace-settings/utils.js

+    fs.writeFileSync(`${codeRepository}/${secrets.app_settings_file}`, settings);
+    fs.writeFileSync(`${codeRepository}/${secrets.flows_file}`, getFormattedFlows(secrets.rp_flows));
+    core.info('Successful');
+    return true;


Do we use this return value?

Not really. Removed.

derickl

This is coming together nicely @kitsao

I've left some feedback inline

derickl · 2022-04-05T12:36:03Z

dynamic-rapidpro-workspace-settings/README.md

+        rp_flows: ${{ secrets.RAPIDPRO_STAGING_FLOWS }}
+        write_patient_state_flow: ${{ secrets.RAPIDPRO_STAGING_WRITE_PATIENT_STATE_FLOW }}
+        app_settings_file: app_settings.json
+        flows_file: flows.js


Is this still relevant? I don't see it in actions.yml

Yes. Added.

derickl · 2022-04-05T12:37:21Z

dynamic-rapidpro-workspace-settings/index.js

+
+(async () => {
+  const githubWorkspacePath = process.env['GITHUB_WORKSPACE'];
+  run(githubWorkspacePath, core, fs);


run is an async method. We should await here

derickl · 2022-04-05T12:40:38Z

dynamic-rapidpro-workspace-settings/package.json

+    "chai": "^4.3.6",
+    "eslint": "^8.12.0",
+    "eslint-plugin-json": "^2.1.2",
+    "fs": "^0.0.1-security",


fs is a core module. Unsure why we are getting it from npm

derickl · 2022-04-05T12:41:44Z

dynamic-rapidpro-workspace-settings/package.json

+  "devDependencies": {
+    "@faker-js/faker": "^6.0.0-beta.0",
+    "@medic/eslint-config": "^1.1.0",
+    "axios": "^0.21.4",


This is a core dependency

derickl · 2022-04-05T12:42:28Z

dynamic-rapidpro-workspace-settings/package.json

+    "rosie": "^2.1.0",
+    "sinon": "^13.0.1",
+    "template-file": "^6.0.1",
+    "util": "^0.12.4"


this is a core dependency

This is a core dependency.

Any library that is used in the core code (i.e not tests) is a core dependency and should be moved to "dependencies"

Are these manually moved to dependencies? Running npm i --save doesn't add them directly.

At this stage, yes.

In principle, dev dependencies are used when developing the tool/code/package and are never referenced when running the code in production

npm however provides two flags that determine where they get saved: --save and --save-dev

derickl · 2022-04-05T12:47:23Z

dynamic-rapidpro-workspace-settings/package.json

+  "description": "dynamic-rapidpro-workspace Shared GitHub Action",
+  "main": "index.js",
+  "scripts": {
+    "eslint": "npx eslint {,**}/*.js {,**}/*.json",


We probably should have the expressions quoted to prevent any interpretation from the underlying shell

derickl · 2022-04-05T12:50:53Z

dynamic-rapidpro-workspace-settings/action.yml

+    required: true
+
+outputs:
+  app-settings: 


Where the these values stored and used?

Outputs should be values used as the program runs. In this case, these are unnecessary. Removed.

derickl · 2022-04-06T07:27:05Z

dynamic-rapidpro-workspace-settings/utils.js

+    const settings = await getReplacedContent(JSON.parse(appSettings), secrets);
+
+    await axios.put(url.href, `"${secrets.rp_api_token}"`);    
+    fs.writeFileSync(`${codeRepository}/${secrets.app_settings_file}`, settings);


can we validate that these paths exist and fail cleanly if not?

A clean fail handled by default.

derickl · 2022-04-06T07:29:23Z

dynamic-rapidpro-workspace-settings/action.yml

+    description: The CHT app-settings file.
+    required: true
+
+  flows_file:


From here, it seems we don't really need the flow file to exist since the contents are loaded from secrets.

Thoughts?

This is simply a file name that would be referenced in other files such as tasks.js.

derickl · 2022-04-06T07:31:05Z

dynamic-rapidpro-workspace-settings/utils.js

+
+    await axios.put(url.href, `"${secrets.rp_api_token}"`);    
+    fs.writeFileSync(`${codeRepository}/${secrets.app_settings_file}`, settings);
+    fs.writeFileSync(`${codeRepository}/${secrets.flows_file}`, getFormattedFlows(secrets.rp_flows));


Are we validating the contents of secrets.flows_file? What happens if you put in a malformed value?
Could we add tests around the same?

That would be similar to validating the extension to be .js. Maybe updating the file name required in other files such as tasks.js is necessary. Looking into that.

I got rid of both flows file and app settings file, and handled them in-code.

derickl

Awesome. Let's put it in use in a project, deploy some code

kitsao added 2 commits March 25, 2022 00:25

action to: update app_settings, set medic-credentials

80ea1db

include missing package

7e40df8

kitsao requested a review from derickl March 24, 2022 21:32

derickl requested changes Mar 25, 2022

View reviewed changes

kitsao added 3 commits March 25, 2022 23:14

feedback

5e2e1b5

extra test

9ca6d17

eslint

96782e2

kitsao requested a review from derickl March 28, 2022 07:43

refactoring tests

ba2f1c7

derickl reviewed Mar 31, 2022

View reviewed changes

no return

2e57d26

derickl requested changes Apr 5, 2022

View reviewed changes

more feedback

250b524

kitsao requested a review from derickl April 5, 2022 20:36

derickl reviewed Apr 6, 2022

View reviewed changes

more feedback

9eb48d9

kitsao requested a review from derickl April 13, 2022 07:04

derickl approved these changes Apr 13, 2022

View reviewed changes

kitsao merged commit 907b0f7 into main Apr 13, 2022

kitsao deleted the add-rapidpro-action branch April 13, 2022 13:42

garethbowen mentioned this pull request Jan 25, 2024

action to update settings for rapidpro workspace medic/cht-core#7274

Closed

5 tasks

		@@ -0,0 +1,26 @@

		const { Factory } = require('rosie');
		const { faker } = require('@faker-js/faker');

action to: update app_settings, set medic-credentials #1

action to: update app_settings, set medic-credentials #1

Conversation

kitsao commented Mar 24, 2022

derickl 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

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

kitsao Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

derickl commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derickl 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

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

derickl left a comment

Choose a reason for hiding this comment

kitsao Mar 25, 2022 •

edited

Loading