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
Update catalog to non-legacy environment names and point to new airflow.openverse.org #4053
Conversation
Full-stack documentation: https://docs.openverse.org/_preview/4053 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
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.
The changes work and I can run the tests locally just as before.
I don't want to block on two notes I left inline that I think were left in by accident.
Co-authored-by: Olga Bulat <obulat@gmail.com>
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 looks good, only one question regarding additional documentation for the ASG/EC2 update case during deployment.
Also, since our production Airflow uses prod
now, we'll need to update this variable too: https://github.com/WordPress/openverse-infrastructure/blob/7a9f8b4e6693463dce1e0afe2dfd6c4d61064aca/ansible/roles/airflow/templates/compose.yml.j2#L11
I believe we can add the variable in the UI and that will take precedence. So we can do that as soon as this PR is merged to ensure no running DAGs are affected by the prod
/production
difference, then remove it once the infrastructure repo is updated.
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
I was trying to understand variable precedence in Airflow. I'm pretty sure environment variables take precendence over UI. When I set This is for connections, but I think variables follow the same logic: https://forum.astronomer.io/t/what-precedence-does-airflow-determine-when-searching-for-a-single-connection/630 Otherwise, yes, we need to update it in the running Airflow instance after merging this, which we can do with the Ansible playbook as you pointed out, and should do in conjunction with merging this. |
Yes I believe you're right, when I've looked into it before I recall seeing that the environment variables were higher than the UI (which is unfortunate because they can't be changed as rapidly!). We'll definitely need to coordinate on that end before merging & deploying. |
Should we switch over to setting variables using airflow CLI instead of environment variables, so that we can have that flexibility? It's easy enough with the Ansible playbook. |
@AetherUnbound It seems like you've looked at this PR a bit, are you able to review it at all? |
Ahh, that's right we could! That would certainly make the experience much more uniform! |
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.
LGTM given the deployment constraints!
Related infrastructure PR: https://github.com/WordPress/openverse-infrastructure/pull/848 |
I haven't deployed this because Freesound is running. @WordPress/openverse-catalog and in particular @stacimc, I'm pretty sure I should not stop it, but it's been running for 8 days 🤔 It is ingesting a lot of records though, after failing a bunch of previous runs. Is that expected? |
@sarayourfriend I've just manually failed that Freesound run in order to load the data we've got so far, so that we can deploy the catalog. You can go ahead with this as well, I'll wait to resume the DAG until you're done! |
To clarify my previous message, we had not yet deployed this (or the routine catalog deploy). We will need to do both; Freesound is paused so we can go ahead whenever. I will handle deploying from here 👍 |
Fixes
Fixes #737 by @AetherUnbound
Related to #2037
Description
Update the catalog code to use
production
instead ofprod
and never usedev
under any circumstances to refer to a deployment environment (references to databases are impossible to fix and must remain). "local" is used to indicate a local development environment. "staging" is essentially unused in catalog runtime code, except for when referring to other resources, and those already use the contemporary, non-legacy names.Realistically this only affects the slack messaging tool, which skip in non-production environments, and pr review reminders, which do the same. This PR updates both to use
production
now.Because this is related to the new Airflow deployment, which uses the unified environment names, I've also gone ahead and added the documentation updates for catalog deployment in this PR. They're small documentation changes and it was easier to group this PR in with that.
Testing Instructions
Read the updated documentation and confirm it all works.
CI should pass, all existing unit tests should pass. The updated code should make sense when reviewed.
I wasn't able to run the tests locally, neither on this branch or on main. Something is going on for me locally where the tests take ages (like literally up to a minute or more per test), so I'm relying on CI to run the tests for me. I am able to run the catalog Airflow instance itself though, and things look to be working fine there. Use
just catalog/up
and visit localhost:9090 to verify Airflow runs without DAG import errors.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin