Skip to content

Conversation

@l-j
Copy link
Contributor

@l-j l-j commented Apr 13, 2023

Resolves #333.

Checklist:

  • support Python 3.11 (only if your adapter's dependencies allow)
  • Consolidate timestamp functions & macros
  • Replace deprecated exception functions
  • Add support for more tests

Works with my dbt projects using both Azure SQL Database and Microsoft SQL Server 2017.

This PR requires changes around incremental_predicates, but as I do not use incremental materialisations much, I do not have time to test everything properly and write tests, so for now I have marked this feature as not implemented. The changes required do not look huge, so I may try to add them later.

I believe, there are some problems with the CI in this repository, but as you can see all the tests are green here:
https://github.com/l-j/dbt-sqlserver/pull/4/checks

@romiof
Copy link

romiof commented May 4, 2023

Hello guys! Any news about this PR?

@mikaelene
Copy link
Collaborator

Looks like we need a CI-container for Python 3.11. Who knows how to get that in here?

@l-j
Copy link
Contributor Author

l-j commented May 5, 2023

@mikaelene My understanding is that with the current Github Actions configuration, we can only have a container for the changes that are already on the master branch (see: https://github.com/l-j/dbt-sqlserver/blob/master/.github/workflows/publish-docker.yml#L9), so a Python 3.11 container will be built after the merge.

And basically, this is what I did to my fork to make CI work. so maybe we should also build it for pull requests to master? If that is an acceptable solution for now, I can prepare a quick pull request with the change. I cannot see the list of maintainers for this repository. @dataders, can you help with that?

@mikaelene
Copy link
Collaborator

Ahh. Get it now. So basically these test will not pass until we merge this to master. Maybe we should create a PR for just building the containers for 3.11? I am a maintainer (and also the creator), but haven't been very active in a while. If you create a PR for just building more versions of the containers, I can approve. It is always easier to merge a PR with green checkmarks.

I haven't used incremental predicates but it looks like a neat feature. But I guess they are not needed to make this release. Maybe should write something in the docs on this.

@l-j
Copy link
Contributor Author

l-j commented May 5, 2023

@mikaelene I think this will do: 03b490c, but I am not an expert in the Gihub Actions.

Now build process is waiting for approval.

And you are right about the documentation. As far as I can see now, the documentation has been moved to the separate repository, so I can prepare a PR there as well, when that PR is accepted.

@l-j
Copy link
Contributor Author

l-j commented May 8, 2023

@mikaelene I see that the step Publish Docker images failed with the following error message:

ERROR: denied: installation not allowed to Write organization package Error: buildx failed with: ERROR: denied: installation not allowed to Write organization package 

And indeed, according to this page, it can be caused by missing permissions.

If we change the trigger rule to allow pull requests to trigger the publication of Docker images, we will need to make additional changes to the repository configuration as described here.

@dataders dataders added this to the v1.4.0 milestone May 10, 2023
@l-j
Copy link
Contributor Author

l-j commented May 12, 2023

After modifying setup.py, we get the following:

The conflict is caused by: dbt-tests-adapter 1.4.1 depends on dbt-core==1.4.1 dbt-sqlserver 1.4.0 depends on dbt-core~=1.4.5 

so I loosen the dev requirements and pin dbt-tests-adapter to the dbt-core version.

@dataders
Copy link
Collaborator

closing because your commits were merged via #350. Thanks @l-j for getting the ball rolling on this btw!

@dataders dataders closed this May 12, 2023
@sdebruyn sdebruyn mentioned this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants