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.

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

Remove use of psql from traffic_ops/db/admin #5327

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MylesBock
Copy link
Contributor

@MylesBock MylesBock commented Nov 24, 2020

What does this PR (Pull Request) do?

Replaces use of psql and dropuser with dburl and lib/pq.
Test coverage is unknown

Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

Run the traffic_ops/db/admin utility against a postgres instance, ensure the multiline SQL queries do not break anything

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

updated dropDB in app/db/admin.go to use pgx+dburl

created connectAndExecute  to deduplicate in app/db/admin.go

replaced all use of psql binary (and dropuser)

wrapped up most querying into a single function (connectAndExecute)

still need to write tests and handle vendoring

added github.com/xo/dburl to vendor.json

added xo/dburl files to vendor

cleaned up a little, added return statement

added dburl/xo MIT license entry to LICENSE

gave some credit where it was due

reintroduced hostport, removed role attributes portion of query
@MylesBock MylesBock changed the title 4954/psql to pgx admin Remove use of psql from traffic_ops/db/admin Nov 24, 2020
@MylesBock MylesBock marked this pull request as draft November 24, 2020 17:53
@MylesBock
Copy link
Contributor Author

Converted to draft, need to figure out what's up with CIAB starting up

@rawlinp rawlinp added database relating to setup/installation/structure of the Traffic Ops database improvement The functionality exists but it could be improved in some way. labels Nov 24, 2020
//todo determine test coverage of this portion
if db, err := dburl.Open(fmt.Sprintf("pg://%s:%s@%s:%s/", DBSuperUser, DBPassword, HostIP, HostPort)); err == nil {
if !returnRows {
if _, err := db.Exec(query); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

So some of our files like seeds.sql contain multiple sql queries. Doesn't this only execute a single sql query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, but in this case it's reading an entire file in for things like patch, seed, create and executing it all at once

This has come up apparently in a few discussions on lib/pq and other repositories related to golang and postgres, and one project offers a solution (https://github.com/gchaincl/dotsql) but it requires more effort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's preferred I could parse in the DML/DDL files and break them down into individual queries

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, I thought db.Exec would return after running only a single query, hence making it not work for files like seeds.sql which have multiple queries. If that works, I'm fine with that -- no need to break them down into individual queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 99% sure this is why ciab doesn't come up in the pipeline though, so I'll break it down into individual queries or use dotsql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database relating to setup/installation/structure of the Traffic Ops database improvement The functionality exists but it could be improved in some way.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'admin' tool should use pq
2 participants