Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Migrate Off Circle CI / To Github Actions + dagger.io #923
Migrate Off Circle CI / To Github Actions + dagger.io #923
Changes from 80 commits
fd6f6f0
795e40a
ff39c5d
1505fc6
44fe33f
2655631
915f67e
f0ef215
e8457df
842466a
0738f2d
277bef1
8d5853d
919528a
15e48fd
ab90c4c
31cb05e
31eceb5
fd54d7f
8de8339
e85232f
fe3300e
b37e14b
51511ec
98607b6
e6ec414
05a2c08
a89ec58
2a18fad
1481396
31b427c
15ba1da
ca33a23
6274d77
0ba91a2
f092026
b968985
8a49567
7723e8d
ea5ebfa
610e5e9
b4411ab
cae6c8a
0c68972
b2f63bd
80eb7e4
4bbfa71
b1d2020
38fda3d
6b599a1
0976c4f
42f2784
4f11291
1384084
543e321
307a9af
f380d46
c334f32
19dcff3
01b0c0c
d3d2844
94af018
72daf90
4f63a3c
b43c9d1
91715d2
943a8dc
3d0dece
080b816
c8477ce
c0a37ae
9b9dc79
8c6a745
6a6b4ce
1ae321a
6361429
6bca5dc
f4293e0
d398065
6108d44
d472f3b
ce92bcf
0c4ed9e
56b14bc
9849c1c
f9a4c58
bbe17a8
3f44e96
afd3866
259ebc7
a8a7010
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ ❤️ ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we get there are we going to also add inputs for
dbt-adapter
anddbt-common
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call out when we update all the adapters to use those packages we should update these to include them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this label still utilized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is the label used? GitHub automatically doesn't run PRs from forks now. Is this leftover from CircleCI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is leftover from before github had a mechanism to prevent running tests from forks maybe? We have this in a bunch of repos, sounds like it can be removed which would be awesome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't seem to use the output from this job anywhere? We should also be able to accomplish the same thing via filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defeats the purpose of using the
pull_request_target
trigger.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you expand on this? Looks like this is what all our other integration test workflows do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read up a bit more adn this does not defeat the purpose of
pull_request
_target` because we're already running the workflow from the head which is actually the point.I understand this is how it's done in the other adapter repos but I'm not sure it's necessary. From the actions/checkout docs
So this feels like a over-complication of checkout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline and we will create a ticket to explain the logic here but leave it as is to follow patterns in adapter repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not using a python matrix here.
matrix.python-version
will be empty and you're just setting it to always use 3.11 here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be tested against all supported python versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the python version being used is actually configured based off the base image dagger uses for the test container as specified here: https://github.com/dbt-labs/dbt-spark/pull/923/files#diff-ca1b9f8fafcab3a8c6df7e520e725035642f58e54df0d88402a7a61fac5578b6R95
Since I don't think we're doing that in CircleCI today I don't think that's a blocker but I agree we should add that
in the near future. Ticket to track: #970
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉