Clean up tox ini #105

JCZuurmond · 2021-02-11T20:22:04Z

As discussed in #103, here some suggestion to change the use of setup.py, requirements.txt and tox.ini. I think we should merge #103 first and sync this with that one.

JCZuurmond · 2021-02-11T20:22:27Z

requirements.txt

@@ -1,9 +1,5 @@
-dbt-core==0.19.0


these were duplicate, i.e. already present in setup.py

JCZuurmond · 2021-02-11T20:23:16Z

requirements.txt

@@ -1,9 +1,5 @@
-dbt-core==0.19.0
-pyodbc>=4.0.27
-azure-identity>=1.4.0
 black~=20.8b1
 pytest-dbt-adapter~=0.4.0
 tox==3.2.0
 flake8>=3.5.0
 certifi==2020.6.20


is this used in the package? Or something like tox and flake, i.e. used when developing the package.

JCZuurmond · 2021-02-11T20:33:15Z

tox.ini

@@ -1,18 +1,6 @@
 [tox]
-skipsdist = True
-envlist = unit, flake8, integration-sqlserver, integration-synapse
+envlist = py37,py38


our test suite is ran for python 3.7 and 3.8

.circleci/config.yml

JCZuurmond · 2021-02-11T20:39:03Z

dev_requirements.txt

@@ -1,9 +1,6 @@
-dbt-core==0.19.0
-pyodbc>=4.0.27
-azure-identity>=1.4.0
 black~=20.8b1


I renamed it to dev_requirements since it are the packages you need to install for local development. Dbt also has a dev_requirements.txt

JCZuurmond · 2021-02-11T20:39:32Z

tox.ini

+[testenv]
+commands = pytest {posargs}
+deps = pytest~=6.2.2 pytest-dbt-adapter~=0.4.0


these are the test dependencies

dev_requirements.txt

.circleci/config.yml

JCZuurmond · 2021-02-11T20:57:21Z

.circleci/config.yml

@@ -6,7 +6,9 @@ orbs:

 jobs:
  integration-sqlserver: &sqlserver
-    docker: 
+    docker:
+      - image: cimg/python:3.7


we need this to run testing for multiple python versions

JCZuurmond · 2021-02-11T21:04:03Z

@swanderz : When I try to install multiple python versions the CI does not work. Could you help?

dev_requirements.txt

dataders · 2021-02-13T23:07:20Z

setup.py

@@ -57,8 +57,8 @@ def _dbt_sqlserver_version():
        ]
    },
    install_requires=[
-        'dbt-core~=0.19.0',
+        "dbt-core>=0.19.0",


I'm fairly certain that we want to keep the tilde character bc (according to this SO answer), it is a combination of to a combination of the following clauses:

>= 0.19.0 and

== 0.19.*

This way a user will get an error if they try to install dbt-core==0.20.0 before we cut a compatible release.

Sure, that is true. Though, we might miss when there is a new minor version and thus forget to leverage that.

The projects I use as examples - like [tox](https://github.com/tox-dev/tox/blob/master/setup.cfg) or [pre-commit](https://github.com/pre-commit/pre-commit/blob/master/setup.cfg) - have lower bounded requirement version. Sometimes you see an upper bound for the major release dbt-core>=0.19.0,<1.0.0.

I think the idea is that dbt-core should be backwards compatible for minor releases, however, it could of course happen that that is not the case.

I will change it back to the ~ for now.

good point. no strong opinion here. @mikaelene might have one though!

dataders · 2021-02-13T23:17:13Z

When I try to install multiple python versions the CI does not work. Could you help?

for "unauthorized" issue, there's a setting that should is already enabled for this repo: Pass secrets to builds from forked pull requests. In the docs for this feature, it does bring up this scenario. Are you by chance "following" your fork from your personal GitHub account rather than from the dbt-msft org? My view looks like below, with dbt-msft in the top left...

Note: If a user submits a pull request to your repository from a fork, but no pipeline is triggered, then the user most likely is following a project fork on their personal account rather than the project itself of CircleCi, causing the jobs to trigger under the user’s personal account and not the organization account. To resolve this issue, have the user unfollow their fork of the project on CircleCI and instead follow the source project. This will trigger their jobs to run under the organization when they submit pull requests.

https://circleci.com/docs/2.0/oss/#example-open-source-projects

dataders

totally on board with this clean up. next steps (as I see them):

pull in Add pytest for unit testing #103
get CircleCI jobs passing
add entry to changelog
get consensus from Mikael and Jeremy on this new approach

.circleci/config.yml

tox.ini

.circleci/config.yml

tox.ini

…r into clean-up-tox-ini

.circleci/config.yml

dataders

I think we're ready to merge. much cleaner now IMHO. @mikaelene -- I approve!

mikaelene

I am not a tox expert. Looks good to me and the tests seems to work. Always good to clean things up :-). We don't have that much code to keep clean.

.circleci/config.yml

JCZuurmond added 14 commits February 11, 2021 21:07

Remove skipdist

7c29fbf

Remove editable flag

90d0eaa

Add space after -r

a0f6098

Remove redundancy of command

089a303

Remove basepython

95ef130

Update dependencies

128b36b

Remove duplicate requirements

769948a

Install requirements.txt in deps

f272aac

Have test env only

b24d54e

Use posargs in circleci

e78bd18

Add pytest to requirements.txt

17f7bda

Install minimally needed packages in tox

a4f740a

Rename requirements to dev_requirements

8b73ad0

Change order of deps

0118899

JCZuurmond commented Feb 11, 2021

View reviewed changes

dev_requirements.txt Outdated Show resolved Hide resolved

Remove pip install from circle ci

0ee10f3

JCZuurmond commented Feb 11, 2021

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

JCZuurmond added 2 commits February 11, 2021 21:50

Make deps multiline

f0cbaa0

Add python images to be sure multiple versions are available

7dec753

JCZuurmond commented Feb 11, 2021

View reviewed changes

JCZuurmond added 2 commits February 11, 2021 21:59

Move images down

b6c8317

Run only for python 37

ce4cd67

JCZuurmond commented Feb 12, 2021

View reviewed changes

dev_requirements.txt Outdated Show resolved Hide resolved

JCZuurmond added 2 commits February 13, 2021 09:19

Fix versions in dev_requirements

85f33c4

Let version in setup.py be lower bounded

0b02e09

dataders reviewed Feb 13, 2021

View reviewed changes

dataders requested changes Feb 14, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

tox.ini Outdated Show resolved Hide resolved

.circleci/config.yml Show resolved Hide resolved

.circleci/config.yml Show resolved Hide resolved

tox.ini Outdated Show resolved Hide resolved

JCZuurmond added 16 commits February 15, 2021 10:15

Remove requirements

2745bd8

Merge branch 'clean-up-tox-ini' of github.com:JCZuurmond/dbt-sqlserve…

1fb8353

…r into clean-up-tox-ini

Revert changes to docker file

c72119c

Add upgrade dbt step

cb25c02

Remove pyodbc image

d9d1717

Remove upgrade dbt

adf4e45

Remove pyodbc from unit job

80c4dd2

Add python image

d12a1e2

Add pip install tox to unit test

de0a9e8

Add sql server to unit job

04681ba

Install odbc

8127bb2

Add sudo

6699419

Reuse parts of other jobs

870b283

Add sudo to python install

1ded9d7

Reset circleci

50c41fa

Upgrade dbt in other jobs

41f3919

JCZuurmond mentioned this pull request Feb 15, 2021

Remove pyodbc docker file #107

Closed

JCZuurmond added 4 commits February 15, 2021 17:43

Remove wrongly pasted step

aa9f4ac

Rename step

5cab4d4

Fix dbt compile

fc8e233

Set back cd

aa8771a

JCZuurmond requested a review from dataders February 15, 2021 16:52

dataders reviewed Feb 15, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

dataders approved these changes Feb 15, 2021

View reviewed changes

Remove executors

5f0bfd2

dataders requested a review from mikaelene February 23, 2021 22:51

mikaelene approved these changes Feb 24, 2021

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

dataders merged commit a298e8b into dbt-msft:master Feb 24, 2021

dataders mentioned this pull request Mar 19, 2021

HOTFIX setup.py doesn't include materializations folder microsoft/dbt-synapse#45

Merged

JustGitting mentioned this pull request Aug 14, 2024

Update on the support and future of this adapter #441

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up tox ini #105

Clean up tox ini #105

JCZuurmond commented Feb 11, 2021

JCZuurmond Feb 11, 2021

JCZuurmond Feb 11, 2021

JCZuurmond Feb 11, 2021 •

edited

Loading

JCZuurmond Feb 11, 2021

JCZuurmond Feb 11, 2021

JCZuurmond Feb 11, 2021

JCZuurmond commented Feb 11, 2021

dataders Feb 13, 2021

JCZuurmond Feb 14, 2021 •

edited

Loading

dataders Feb 14, 2021

dataders commented Feb 13, 2021

dataders left a comment

dataders left a comment

mikaelene left a comment

Clean up tox ini #105

Clean up tox ini #105

Conversation

JCZuurmond commented Feb 11, 2021

JCZuurmond Feb 11, 2021

Choose a reason for hiding this comment

JCZuurmond Feb 11, 2021

Choose a reason for hiding this comment

JCZuurmond Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

JCZuurmond Feb 11, 2021

Choose a reason for hiding this comment

JCZuurmond Feb 11, 2021

Choose a reason for hiding this comment

JCZuurmond Feb 11, 2021

Choose a reason for hiding this comment

JCZuurmond commented Feb 11, 2021

dataders Feb 13, 2021

Choose a reason for hiding this comment

JCZuurmond Feb 14, 2021 • edited Loading

Choose a reason for hiding this comment

dataders Feb 14, 2021

Choose a reason for hiding this comment

dataders commented Feb 13, 2021

dataders left a comment

Choose a reason for hiding this comment

dataders left a comment

Choose a reason for hiding this comment

mikaelene left a comment

Choose a reason for hiding this comment

JCZuurmond Feb 11, 2021 •

edited

Loading

JCZuurmond Feb 14, 2021 •

edited

Loading