-
Notifications
You must be signed in to change notification settings - Fork 214
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
Simplify and reorder parameters to catalog/pgcli
recipe
#2023
Simplify and reorder parameters to catalog/pgcli
recipe
#2023
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.
LGTM.
It may be best to even move this recipe up to the root file considering both databases are shared across multiple apps. just api/dbshell
will get you the API database.
Something like just dbshell catalog
and just dbshell api
would be pretty handy!
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 was under the impression that the host and port were the parameters most often to change, but I haven't used this recipe enough to know.
Since the api
also has a pgcli
recipe could you copy the same changes over there?
@sarayourfriend's suggestion to have a pgcli
recipe in the root makes sense, considering they have the same basic structure (and they both ultimately use ../exec
recipe from the root justfile
anyway).
But I'd still prefer to retain the recipes api/pgcli
and catalog/pgcli
(that call the root pgcli
with the right defaults) to keep with the <project>/<recipe>
convention instead of the <recipe> <project>
convention (that I've seen in the infra repo).
Great call with both suggestions! I'll set up a common recipe at the top level, but then use it in |
@dhruvkb and @sarayourfriend - I've combined recipes where possible, how does this look? |
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.
Right on!
Description
This PR adjust the recipe for
catalog/pgcli
to reduce the number of overrides necessary to define when switching between the Airflow and upstream databases. Additionally, I've reordered the parameters (since withjust
you have to override all parameters in order if you want to change one) so that the fewest need to be changed. Our username and passwords are the same for both databases so I just combined the field.Lastly, the current config actually didn't work because it was using the airflow creds to connect to the upstream database (which it didn't have access to). I can only speak from personal experience, but almost every time I use the
pgcli
command for the catalog it's to access the upstream database, so I changed the default to be that.Update: Based on feedback below, I've combined the recipes into a
_pgcli
recipe at the root level, which both the API and catalog now call.Testing Instructions
main
, runningj catalog/pgcli
and thenselect * from image limit 10
should raise an errorj catalog/pgcli airflow airflow
and run\d
and various selects to ensure you can see the airflow schemaj api/pgcli
and confirm that this correctly connects to the API's database and can be used to query the API as well.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin