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

updated docs functionality to account for dataclasses refactor #1716

Conversation

talagluck
Copy link

Following up on this PR: 1713

This code has been refactored to account for the new dbt dataclasses re-factor.

@cla-bot cla-bot bot added the cla:yes label Aug 30, 2019
@talagluck
Copy link
Author

Hi @drewbanin. I believe CLA stuff should be in order as well? I changed my primary github email to match the info on the CLA, but let me know if you have any additional issues.

Thanks!

@drewbanin
Copy link
Contributor

Thanks @talagluck! I'm kicking off the test suite now. I do expect that there will be many tiny changes required to the unit/integration test suite - lmk if you have any questions about how to resolve those once the test results roll in :)

@@ -7,7 +7,7 @@

class SourceConfig:
AppendListFields = {'pre-hook', 'post-hook', 'tags'}
ExtendDictFields = {'vars', 'column_types', 'quoting', 'persist_docs'}
ExtendDictFields = {'vars', 'column_types', 'quoting', 'persist_docs', 'docs'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this line failed for pep8 reasons:

core/dbt/source_config.py:10:80: E501 line too long (82 > 79 characters)

Copy link
Author

Choose a reason for hiding this comment

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

Just fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks! Tests are running now

@drewbanin
Copy link
Contributor

Hey @talagluck - you should be able to see a list of failed unit and integration tests now. There are a good number of them, but the fixes are all really tiny - you'll just need to add an empty docs dict to each of the "expected" data structures in the tests.

Definitely let me know if you'd like a hand or some pointers! To run the unit / integration tests locally, check out our contributing guide

@talagluck
Copy link
Author

Hi @drewbanin. I fixed a few of the tests and committed without thinking, so I'm waiting for the CI to complete again.

I tried setting up an environment to run tests locally and got this error when trying to run PGHOST=localhost PGUSER=root PGPASSWORD=password PGDATABASE=postgres bash test/setup_db.sh:

+ grep '^PG'
PGUSER=root
PGPASSWORD=password
PGDATABASE=postgres
PGHOST=localhost
+ PGUSER=root
+ export PGUSER
+ PGPORT=5432
+ export PGPORT
+ PGHOST=localhost
+ [[ -n '' ]]
+ createdb dbt
createdb: could not connect to database template1: FATAL:  role "root" does not exist
+ psql -c 'CREATE ROLE root WITH PASSWORD '\''password'\'';'
psql: FATAL:  role "root" does not exist
+ psql -c 'ALTER ROLE root WITH LOGIN;'
psql: FATAL:  role "root" does not exist
+ psql -c 'GRANT CREATE, CONNECT ON DATABASE dbt TO root WITH GRANT OPTION;'
psql: FATAL:  role "root" does not exist
+ psql -c 'CREATE ROLE noaccess WITH PASSWORD '\''password'\'' NOSUPERUSER;'
psql: FATAL:  role "root" does not exist
+ psql -c 'ALTER ROLE noaccess WITH LOGIN;'
psql: FATAL:  role "root" does not exist
+ psql -c 'GRANT CONNECT ON DATABASE dbt TO noaccess;'
psql: FATAL:  role "root" does not exist
+ set +x

@drewbanin
Copy link
Contributor

Hey @talagluck - just kicked off the tests again.

Re: that error: I think you may need to run:

docker-compose up -d database

via the contributing guide. Let me know if you've already tried that!

@talagluck
Copy link
Author

Thanks, @drewbanin! I did do that. I basically followed the instructions line by line until that point.

@drewbanin
Copy link
Contributor

huh! And you're still not able to get it working?

I don't think you should need to do this, but maybe you can try running:

PGHOST=localhost PGUSER=root PGPASSWORD=password PGDATABASE=postgres createuser root

This should create the root user, which should allow the rest of the commands to succeed. Let me know if that fixes it - we might need to update the docs?

@talagluck
Copy link
Author

Hi @drewbanin. I ran createuser root right before PGHOST=localhost PGUSER=root PGPASSWORD=password PGDATABASE=postgres and that worked! I'll let you know if I have any more trouble.

@talagluck
Copy link
Author

@drewbanin, I've updated some more tests based off of the results of the CI. I've been having a bunch of trouble getting my environment set up for proper testing, so I did it sort of blindly. Would you have some time today or tomorrow to pair on figuring out that devloop?

Thanks!

@drewbanin
Copy link
Contributor

Sure thing @talagluck - feel free to drop me a line on Slack - happy to help you get this sorted.

@talagluck
Copy link
Author

Hi @drewbanin, I've made some updates to the tests and a minor update the NodeConfig dataclass, and it seems as though all the tests have passed! Could we rerun CircleCI?

@drewbanin
Copy link
Contributor

@talagluck should be running now!

@drewbanin
Copy link
Contributor

hey @talagluck - looks like the unit tests are passing! I can see some failures on the Postgres integration tests, and I bet there are other minor test tweaks to make around Snowflake/BigQuery/Redshift too.

Let me know if there's anything I can help out with here!

@talagluck
Copy link
Author

Thanks, @drewbanin! I was out last week and now I'm catching up on things, but I'm hoping to get back to this tomorrow. I might have a slight issue running the Postgres tests, and I'll let you know tomorrow if I can't figure anything out.

@talagluck
Copy link
Author

Hi @drewbanin - I'm having some trouble getting a testing postgres environment to work on my machine, and also understanding the failing errors. Do you have a few minutes to pair today?

@talagluck
Copy link
Author

Hi @drewbanin, I'm back from parental leave, and I'm looking to finish this up. I'm going to go to the #development channel of dbt slack for broader questions, but wanted to check in here to find out what branch I should rebase from/to at this stage.

Thanks!

@drewbanin
Copy link
Contributor

hey @talagluck - welcome back! We're still on dev/louisa-may-alcott, but we're wrapping up that release within the next couple of weeks. I'd love to slip this change in, but if not, we can definitely target it for the 0.15.1 release shortly thereafter. Either way, dev/louisa-may-alcott is going to be the move for the time being :)

@talagluck
Copy link
Author

Hi @drewbanin, apologies - this keeps getting away from us. Where do things stand now? What branch would it be best to work from at this point?

Thanks!

@drewbanin
Copy link
Contributor

hey @talagluck - this can go out in our 0.16.0 release - you'll want to change the base branch to dev/barbara-gittings and pull in any new commits as required!

@drewbanin drewbanin changed the base branch from dev/louisa-may-alcott to dev/barbara-gittings January 7, 2020 02:19
@drewbanin
Copy link
Contributor

@talagluck I'm going to close this PR out as stale, but will happily re-open if you're interested in picking it back up!

@drewbanin drewbanin closed this Jan 30, 2020
@talagluck
Copy link
Author

Thank you @drewbanin! I've been out on leave and just came back. Apologies for the delays on this. I'll let you know when we have the resources to jump back in!

@gshank gshank mentioned this pull request Jan 10, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants