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

Rewriting the action.yml to no longer use the DOCKERFILE #6

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

Aostojic
Copy link
Contributor

Rewrote the action,yml workflow so that it installs pre-commit and dbt directly in the github runner so that the pre-commit arguments can be properly passed to pre-commit

@codecov-commenter
Copy link

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (9ff7bc9) compared to base (53d2f34).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #6   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           44        44           
  Lines         1971      1971           
  Branches       256       256           
=========================================
  Hits          1971      1971           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@JFrackson JFrackson left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'd want to add some documentation or modify the doc in the README to indicate this. Can you update that and request another review?

Comment on lines +24 to +28
dbt-core==1.0.0 \
dbt-postgres==1.0.0 \
dbt-redshift==1.0.0 \
dbt-snowflake==1.0.0 \
dbt-bigquery==1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to install every connector? And all version 1.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this action can be used to run some dbt commands via the hooks, ie dbt-test, I believe we need the connectors for that. Maybe we should be installing the latest version of dbt though, any thoughts on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, ideally we would need to install every connector and it could be passed as a config. For now, however, I think it's probably fine to keep it like this and install them here. I'd bump to 1.2 for all of them though just to be closer to latest

@Aostojic
Copy link
Contributor Author

This looks good to me, but I'd want to add some documentation or modify the doc in the README to indicate this. Can you update that and request another review?

What kind of additional documentation where you looking for, I'm not sure how to best address this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants