cleaned up env.js #1080
cleaned up env.js #1080
Conversation
|
results: |
|
I am going to remove the unused dependency (knex) |
There was a problem hiding this comment.
Still some questionable variables in the code. Also the documentation in the README.md need to be updated to reflect all the changes here: https://github.com/openaq/openaq-fetch/blob/main/README.md#installing--running
and here
https://github.com/openaq/openaq-fetch/blob/main/docs/env.md
I'm also curious how index.adapter.sh, index.sh, run-script.js, docker-compose.yml are used if at all. If not used they need to be removed as well.
Is anything is data_scripts being run with the new process? they seem like most legacy adapters.
src/lib/env.js
Outdated
| * @param {string} psqlPassword | ||
| * @param {string} psqlDatabase | ||
| * @param {number} psqlPoolMin | ||
| * @param {number} psqlPoolMax | ||
| * @param {string} apiURL |
There was a problem hiding this comment.
I dont believe fetch communicates with the API any more, this and any functionality that is using this is likely stale, re-review this usage
There was a problem hiding this comment.
API_URL is used in deploy.yml for GH secrets
src/lib/env.js
Outdated
| * @param {string} psqlPassword | ||
| * @param {string} psqlDatabase | ||
| * @param {number} psqlPoolMin | ||
| * @param {number} psqlPoolMax | ||
| * @param {string} apiURL | ||
| * @param {string} webhookKey |
There was a problem hiding this comment.
Are we using the webhook still? I dont believe so. This an its usage needs to be reviewed and removed if needed
There was a problem hiding this comment.
webhook key is being called by fetch.js, which in turn is being called by index.js
src/lib/env.js
Outdated
| @@ -181,13 +174,6 @@ export default () => { | |||
| const maxParallelAdapters = +_env.MAX_PARALLEL_ADAPTERS || 1024; | |||
There was a problem hiding this comment.
Is this still relevant now thats its on lambda? seems like something from the container version
There was a problem hiding this comment.
maxParallelAdapters is being used in fetch.js and is included in our .env files
There was a problem hiding this comment.
Hard to say - the impression that i get is that it will default to 2x the cores based on the documentation for the python version. It was set to 1 in production though.
|
I have removed all of the unused files, I need to investigate the use of API_URL in deploy.yml to fetch GH secrets |
Ignore anything in github action. All this clean up is agnostic of whats happning in Github Actions |
|
@majesticio Where are we standing on this branch? |
|
This should be good to go, but I will need to adjust it to accommodate our recent changes |
|
My comments were never addressed re:API_URL and WEBHOOK_KEY These are not actively used and need to be cleaned up |
|
I will get those taken care of. |
|
|
There was a problem hiding this comment.
This one does not seem ready. I ran it locally and it did not finish and because there are so many changes it makes me nervous to merge it into main.
Could you (@majesticio) run it locally and compare it to a local run on the main branch and let me konw what you think?
|
@caparker it ran identically for me, and finished a few seconds faster. I am concerned if it does not work on your end |
|
I’ll take a closer look on my end. May have just been a fluke .
…On Fri, Mar 22, 2024 at 11:59 AM Gabriel Fosse ***@***.***> wrote:
@caparker <https://github.com/caparker> it ran identically for me, and
finished a few seconds faster. I am concerned if it does not work on your
end
—
Reply to this email directly, view it on GitHub
<#1080 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOE5OWL6TXJ3S5IRDW22U3YZR5SHAVCNFSM6AAAAABDATO62WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJVG4ZDQOJRHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
cleaned up env.js and removed knexfile Dockerfile and associated docs