Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(#72): Replace hardcoded values with env variables #73

Merged
merged 12 commits into from
Feb 26, 2024
Merged

feat(#72): Replace hardcoded values with env variables #73

merged 12 commits into from
Feb 26, 2024

Conversation

andrablaj
Copy link
Member

Closes #72

@andrablaj andrablaj self-assigned this Feb 21, 2024
@andrablaj
Copy link
Member Author

andrablaj commented Feb 21, 2024 •
edited
Loading

@lorerod @njuguna-n I tried to do similar changes as you did for BRAC Models. I didn't manage to get the unit tests pass, though. Do they pass for BRAC Models?

There is still a v1 hardcoded value in couchdb.sql that I didn't manage to replace by an env variable successfully at the moment.

@andrablaj andrablaj marked this pull request as draft February 21, 2024 17:56
@andrablaj andrablaj changed the title feat: Replace hardcoded values with env variables feat(#72): Replace hardcoded values with env variables Feb 21, 2024
@lorerod
Copy link
Contributor

lorerod commented Feb 21, 2024

I didn't manage to get the unit tests pass, though.

@andrablaj, the unit tests are passing on CI. Do you mean locally?

@andrablaj
Copy link
Member Author

andrablaj commented Feb 22, 2024 •
edited
Loading

@lorerod, sorry for not being clear! Yes, I meant that the tests fail when I run them locally. Here's the error:

Encountered an error:
Parsing Error
Env var required but not provided: 'DBT_POSTGRES_USER'

I get the same error for the main branch too when I run the tests locally.

@witash confirmed that he is not encountering the error when running the unit tests locally for main and hardcoded-variables branches.

@andrablaj andrablaj marked this pull request as ready for review February 22, 2024 10:52
@andrablaj andrablaj requested a review from witash February 22, 2024 11:36
@lorerod
Copy link
Contributor

lorerod commented Feb 22, 2024

@andra, hi!

I get the same error for the main branch too when I run the tests locally.

How are you running the tests locally?
I checked out this branch, and it is working for me locally. You must execute the three shell scripts setup every time you run the test: setup.sh, run_dbt_tests.sh, and tear_down.sh.

Env var required but not provided: 'DBT_POSTGRES_USER'

This env var is setup in this line of setup.sh

Let me know if it works for you.

@andrablaj
Copy link
Member Author

@lorerod I tried the steps in the README, and double-checked my env variables, and they seem to align with what's needed to run the tests. As the CI passes and both you and Tom manage to run the tests locally, can you please review the PR and share comments, if any, the time I fix my local env? Thank you!

njuguna-n
njuguna-n reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@njuguna-n njuguna-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few minor comments

.github/workflows/test.yml Outdated Show resolved Hide resolved
models/root/couchdb.sql
@@ -12,4 +10,4 @@
SELECT
doc->>'type' AS type,
*
FROM {{ import_couchdb_data }}
FROM v1.{{ env_var('POSTGRES_TABLE') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the schema defined in env variables here as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned here, I didn't manage to get that working. I will try again tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, replacing v1 with {{ env_var('POSTGRES_SCHEMA') }} throws a dbt.medic table does not exist error. I will leave this hardcoded for now to have the pipeline running correctly, and will open a separate issue to remove the hardcoded v1.

models/root/root.yml Outdated Show resolved Hide resolved
packages.yml Outdated Show resolved Hide resolved
lorerod
lorerod previously requested changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@lorerod lorerod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @andrablaj! Thank you for making the improvements. I have a few minor suggestions to make.

Before we merge this to the main branch, I think we need to ensure that the cht-sync tests pass with this version of cht-pipeline, right @njuguna-n?
I can check when you have a final version of this, and if the tests are not passing, I will create an issue on that repository and make sure this is mentioned as a dependency.
What do you think?

models/root/root.yml Outdated Show resolved Hide resolved
tests/check_tests_coverage.sh Show resolved Hide resolved
tests/run_dbt_tests.sh Show resolved Hide resolved
@njuguna-n
Copy link
Contributor

@lorerod yes, I agree that we need to have tests passing.

@andrablaj andrablaj mentioned this pull request Feb 23, 2024
Merged
@andrablaj
Copy link
Member Author

andrablaj commented Feb 23, 2024 •
edited
Loading

Thanks both for your comments!

@lorerod I ran the cht-sync e2e tests towards this cht-pipeline branch, and they are passing after doing a few changes. The changes can be found in this cht-sync branch. Please note that the test.yml file links to this branch for the purpose of testing and having the CI pass, but that needs to be changed back to the main branch before merging.

@andrablaj andrablaj requested a review from lorerod February 23, 2024 10:36
@andrablaj andrablaj requested a review from njuguna-n February 23, 2024 10:59
@witash witash mentioned this pull request Feb 23, 2024
Closed
@andrablaj andrablaj removed the request for review from lorerod February 26, 2024 15:16
@andrablaj andrablaj dismissed lorerod’s stale review February 26, 2024 15:17

Lorena OOO until mid March. Her suggestions were all addressed.

@andrablaj andrablaj merged commit 3dfa8e7 into main Feb 26, 2024
2 checks passed
@andrablaj andrablaj deleted the hardcoded-variables branch February 26, 2024 15:17
medic-ci pushed a commit that referenced this pull request Aug 27, 2024
@semantic-release-bot
chore(release): 1.0.0 [skip ci]
b743158 
# 1.0.0 (2024-08-27)

### Bug Fixes

* **#145:** docker compose instead of docker-compose ([b4ecea6](b4ecea6)), closes [#145](#145)

### Features

* **#148:** add automatic releases and versioning ([#152](#152)) ([52cf12a](52cf12a)), closes [#148](#148)
* **#72:** Replace hardcoded values with env variables ([#73](#73)) ([3dfa8e7](3dfa8e7)), closes [#72](#72)
@medic-ci
Copy link

🎉 This issue has been resolved in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njuguna-n njuguna-n njuguna-n approved these changes

@lorerod lorerod lorerod left review comments

@witash witash Awaiting requested review from witash

Assignees

@andrablaj andrablaj

Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace harcoded values with environment variables in root.yml
4 participants
@andrablaj @lorerod @njuguna-n @medic-ci