-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add flexibility to ArcGIS Feature Services connector #1054
Conversation
e67adb2
to
dad0e5c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1054 +/- ##
==========================================
+ Coverage 55.48% 56.55% +1.06%
==========================================
Files 97 97
Lines 4837 4905 +68
Branches 760 775 +15
==========================================
+ Hits 2684 2774 +90
+ Misses 2018 1998 -20
+ Partials 135 133 -2 ☔ View full report in Codecov by Sentry. |
2640dc9
to
7288b16
Compare
7288b16
to
7cd42f5
Compare
7cd42f5
to
b7da50d
Compare
Just rebased to fix clunky language in a docstring, no code changes since the linked action running all of these |
064d83f
to
f270bab
Compare
@@ -20,7 +40,8 @@ def test_config_source_type(): | |||
assert c.source.url | |||
|
|||
|
|||
def test_config_version_socrata(): | |||
@patch("requests.get", side_effect=mock_request_get) |
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.
a tad unrelated to this PR but included because I was touching library tests with arcigs fs stuff - this was making live calls to socrata before, now it's using a mocked api response
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.
These I looked at the feature server linked in the info section to make sure the changes being made were accurate
Build is failing due to duplicate rows in a source dataset for templatedb (failing a dbt check). This is still ready to go! |
] | ||
|
||
|
||
def resolve_layer( |
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 refactored this just slightly - seemed to be a bit cleaner with a match ... case
. See latest commit
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.
LGTM!
Annoyed at this (see final comments especially) |
@fvankrieken got some builds failing and seems related to this GFT and FacDB have the same error while planning the build: ValidationError: 1 validation error for Config
dataset.source.arcgis_feature_server.layer
Extra inputs are not permitted [type=extra_forbidden, input_value=0,
input_type=int] for GFT, it's due to |
Oh shoot. Blanked about mutating existing structures in those config objects. I'll get a fix |
(note from in-person chat) this action run to archive all ArcGIS Feature Server datasets may fix it |
Part of #1004, and touches a bit on #923
Thought updating ArcGIS Feature Service datasets for GFT was going to be simple. Had a bit of a wrench thrown in when multiple attempts failed
I broke out the commits so that the first commit has a lot of the test data (big part of the added lines in this PR), so commit-by-commit will be easier.
Context
As a refresher for terms
So our library datasets refer to a single layer of a given Feature Server - in some cases, we have a couple datasets per feature server. We identified these in our templates with a
layer_id
(or assumed it to be0
.) Turns out layer ids aren't necessarily constant for a given feature server. I think ideally they are supposed to be, but for a handful of layers, the maintainers seemed to delete the previous layer and simply create a new one (with a new id).Given that, I thought it made sense to
There's a strong argument that generally, it's safer to specify id rather than name... but I don't know if that's something we can rely on here. I would hope so, but it's hard to tell.
Summary of changes
Separated out logic of models a little bit. Before, we just had a not quite accurately named
FeatureServer
object, shared across library and the connector. I separated things out so we haveSeparating these out made the different utilities a lot cleaner - not having to worry about if the layer object had its id and name resolved yet.
Beyond that, added and expanded FeatureService utils and added tests, which hadn't existed before
Test run
Passing run of calling library for all datasets