-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
action to update settings for rapidpro workspace #7274
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.
some quick feedback.
Glad to see this coming together @kitsao
with: | ||
directory: 'my_app_folder' | ||
hostname: myapp.staging.company.org | ||
username: ${{ secrets.STAGING_USERNAME }} |
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.
Consider prefixing the secret variables with RAPIDPRO_
to avoid using the wrong secrets
|
||
- name: update-medic-credentials | ||
run: | | ||
curl -X PUT https://${{ inputs.username }}:${{ inputs.password }}@${{ inputs.hostname }}/_node/couchdb@127.0.0.1/_config/medic-credentials/${{ inputs.value_key }} -d '"${{ inputs.token }}"' |
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.
CouchDB would not be running on 127.0.0.1
. Did you intend to use ${{inputs.hostname}}
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. Should point to the local COUCH_NODE_NAME
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.
Looks good overall. Some improvement recommendations shared in-line.
|
||
- name: update-app-settings | ||
run: | | ||
jq 'walk(if type == "object" and has("base_url") then .base_url = "${{ inputs.url }}" else . end) | walk(if type == "object" and has("value_key") then .value_key = "${{ inputs.value_key }}" else . end) | walk(if type == "object" and has("flow") then .flow.expr = "${{ inputs.write_patient_state_flow }}" else . end) | walk(if type == "object" and has("groups") then .groups.expr = "[${{ inputs.group }}]" else . end)' app_settings.json > app_settings.json.tmp && cp app_settings.json.tmp app_settings.json && rm app_settings.json.tmp |
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.
It's possible to write app_settings.json directly
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.
is jq
the best way to do this? i suspect you'd really improve readability if this was (for example) a node script
const appSettings = require('./app_settings.json');
.. some stuff to generate app_settings.json
description: The password for user with name "inputs.username" | ||
required: true | ||
|
||
url: |
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.
Recommend renaming to rapid_pro url to avoid confusion
description: The username of an administrator account on the instance at "inputs.hostname". This user will make the deployment. | ||
required: true | ||
|
||
password: |
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.
hostname, username & password can be reused from deploy-with-medic-conf
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.
Maybe, if the two jobs are used together.
|
||
flows: | ||
description: A flows.js file to be used in compile-app-settings | ||
value: ${{ steps.generate-flows-js.outputs }} |
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.
flows.js generated in L83
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
# update-rapidpro-workspace-settings Shared GitHub Action | ||
The `update-rapidpro-workspace-settings` is a parameterised reusable GitHub action that updates app-settings prior to compiling and uploading to a running CHT instance. | ||
|
||
This can be executed jointly with the deployment Github action. |
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 can be executed jointly with the deployment Github action. | |
This can be executed jointly with other Github actions like [deploy-with-medic-conf](https://github.com/medic/cht-core/tree/master/.github/actions/deploy-with-medic-conf) |
directory: 'my_app_folder' | ||
hostname: myapp.staging.company.org | ||
couch_node_name: myapp.couchdb.node.name | ||
username: ${{ secrets.CHT_STAGING_USERNAME }} |
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.
username: ${{ secrets.CHT_STAGING_USERNAME }} | |
couch_username: ${{ secrets.CHT_STAGING_USERNAME }} |
hostname: myapp.staging.company.org | ||
couch_node_name: myapp.couchdb.node.name | ||
username: ${{ secrets.CHT_STAGING_USERNAME }} | ||
password: ${{ secrets.CHT_STAGING_PASSWORD }} |
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.
password: ${{ secrets.CHT_STAGING_PASSWORD }} | |
couch_password: ${{ secrets.CHT_STAGING_PASSWORD }} |
couch_node_name: myapp.couchdb.node.name | ||
username: ${{ secrets.CHT_STAGING_USERNAME }} | ||
password: ${{ secrets.CHT_STAGING_PASSWORD }} | ||
url: my.rapidpro.workspace.url |
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.
url: my.rapidpro.workspace.url | |
rp_hostname: my.rapidpro.workspace.url |
username: ${{ secrets.CHT_STAGING_USERNAME }} | ||
password: ${{ secrets.CHT_STAGING_PASSWORD }} | ||
url: my.rapidpro.workspace.url | ||
token: ${{ secrets.RAPIDPRO_STAGING_TOKEN }} |
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.
token: ${{ secrets.RAPIDPRO_STAGING_TOKEN }} | |
rp_api_token: ${{ secrets.RAPIDPRO_STAGING_TOKEN }} |
url: my.rapidpro.workspace.url | ||
token: ${{ secrets.RAPIDPRO_STAGING_TOKEN }} | ||
value_key: medic.credentials.key | ||
group: ${{ secrets.RAPIDPRO_STAGING_GROUP }} |
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.
group: ${{ secrets.RAPIDPRO_STAGING_GROUP }} | |
rp_contact_group: ${{ secrets.RAPIDPRO_STAGING_GROUP }} |
token: ${{ secrets.RAPIDPRO_STAGING_TOKEN }} | ||
value_key: medic.credentials.key | ||
group: ${{ secrets.RAPIDPRO_STAGING_GROUP }} | ||
flows: ${{ secrets.RAPIDPRO_STAGING_FLOWS }} |
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.
flows: ${{ secrets.RAPIDPRO_STAGING_FLOWS }} | |
rp_flows: ${{ secrets.RAPIDPRO_STAGING_FLOWS }} |
|
||
- name: update-app-settings | ||
run: | | ||
jq 'walk(if type == "object" and has("base_url") then .base_url = "${{ inputs.url }}" else . end) | walk(if type == "object" and has("value_key") then .value_key = "${{ inputs.value_key }}" else . end) | walk(if type == "object" and has("flow") then .flow.expr = "${{ inputs.write_patient_state_flow }}" else . end) | walk(if type == "object" and has("groups") then .groups.expr = "[${{ inputs.group }}]" else . end)' app_settings.json > app_settings.json.tmp && cp app_settings.json.tmp app_settings.json && rm app_settings.json.tmp |
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.
is jq
the best way to do this? i suspect you'd really improve readability if this was (for example) a node script
const appSettings = require('./app_settings.json');
.. some stuff to generate app_settings.json
@@ -0,0 +1,60 @@ | |||
# update-rapidpro-workspace-settings Shared GitHub Action | |||
The `update-rapidpro-workspace-settings` is a parameterised reusable GitHub action that updates app-settings prior to compiling and uploading to a running CHT instance. |
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 don't think this summary does a particularly thorough job of explaining what is happening here.
- Who is this relevant for? This is relevant only for CHT apps that have either outbound integrations with RP, or which have logic which requires RP guids in the flows. Is that right?
- Introduce the problem? I've got a dev and a production rapidpro workspace. I want my dev deployment to use the dev workspace and my prod deployment to use the prod workspace. Is that accurate?
- What are the steps you should go through to generate the dev flow.js (can they just run the script below?). Given a very basic rapidpro setup - can you provide a sample flow.js file?
- Users work with a dev version of a "flow.js" file in their repository. This script replaces the "flow.js" file with an auto-generated file created from data returned by the RP API
- What attributes are expected in your app_settings.json if any?
@@ -0,0 +1,60 @@ | |||
# update-rapidpro-workspace-settings Shared GitHub Action |
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.
# update-rapidpro-workspace-settings Shared GitHub Action | |
# dynamic-rapidpro-workspace Shared GitHub Action |
Nit - maybe rename to be about the problem that is being solved rather than the action that the tool is doing?
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.
Agreed.
@kennsippell, I have following script in the same folder as the
I tested and works. I am not sure how to run it since |
It does @kennsippell. From a first attempt, it requires committing |
|
If the commit with the node_modules is the most recent, you can run the following to unstage the folder
Install GIt LFS from https://git-lfs.github.com
You'll may need to update your action to checkout with 'lfs' like actions/checkout#270 (comment) |
After carefully reworking this, I am blocked by See playground action here. Looks like I so far have two options:
Thoughts @kennsippell @derickl @eljhkrr |
Resolving the full path to the project root folder did the trick cc @derickl. On to the next, have to perform |
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.
@kitsao who are you engaging from cht-core team on this? You will need sign-off from that group.
Normally, you would want to a staging deployment use the staging workspace and a production deployment to use the corresponding production workspace. This action automates the process of updating `app_settings.json` to use relevant UUIDs. | ||
|
||
### How it works | ||
1. It adds the configured `value_key` to CouchDB’s config storage to securely store the [credentials](https://docs.communityhealthtoolkit.org/apps/reference/app-settings/outbound/#credentials). |
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 don't get why this is required... shouldn't the CouchDB credentials be setup once? why do they need to change with code deployment?
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.
value_keys
might not always be the same, could be someone's preference.- We do this to be sure that we have the right credentials for each push to production. For instance, more recently in I-TECH Aurum, we found out that the the
value_key
in app settings was not matching that in CouchDB and we couldn't track to when the change occurred.
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 would recommend removing this from the script for two reasons.
-
that's a weird story with I-Tech Aurum, but i'm not sure that setting the credentials in github secrets is superior than setting it in couchdb itself. the potential for password admin problems is the same, you've just moved it to a different place. or have i missed something?
-
i'd prefer actions which do one thing well, rather than cluttering the interface with details required for two coupled but unrelated activities.
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.
You might have missed something. The password given in secrets is set in CouchDB here.
const flowsContent = `module.exports = ${rp_flows};`; | ||
|
||
setMedicCredentials(couch_username, couch_password, hostname, couch_node_name, value_key, rp_api_token); | ||
replace(options); |
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-wide replacement seems dangerous. you're string replacing even across the compiled javascript code? how can this be more clearly targeted to only relevant areas of config. should this run on base_config
before running deployment? should this be limited to specific branches of the JSON tree?
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 action should be executed before other actions that is is jointly executed with such as deploy-with-medic-conf
, so that dependent pieces of code are updated prior to compiling app settings. Otherwise, if executed later, we'll end up with broken updates.
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.
so doesn't this have to operate on base_settings
or the code you replace in app_settings will simply be overwritten by the next compile-app-settings during deployment?
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.
It looks like this is obsolete (solved by medic/gh-actions#1 ). Closing - please reopen if I've missed the point! |
Description
Auto-deployment update for RapidPro deployments as a parameterized and re-usable GitHub action
medic/app-services-team#165
Code review checklist
readme
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.