Extract snapshot lib #10164
Extract snapshot lib #10164
Conversation
LocalStack Community integration with Pro 2 files 2 suites 1h 20m 38s ⏱️ Results for commit 2a7be19. ♻️ This comment has been updated with latest results. |
dominikschubert
added
the
semver: patch
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 12s ⏱️ Results for commit 2a7be19. ♻️ This comment has been updated with latest results. |
dominikschubert
requested review from
MEPalma,
alexrashed,
baermat,
bentsku,
calvernaz,
dfangl,
giograno,
joe4dev,
macnev2013,
pinzon,
simonrw,
steffyP,
thrau and
viren-nadkarni
as code owners
dominikschubert
removed request for
alexrashed,
bentsku,
calvernaz,
dfangl,
giograno,
joe4dev,
macnev2013,
pinzon,
simonrw,
steffyP and
viren-nadkarni
thrau
left a comment
thrau
left a comment
There was a problem hiding this comment.
nice work! can't wait to start using the lib in other projects :-)
dominikschubert
force-pushed
the
snapshot-lib-extraction
branch
3 times, most recently
from
9057684 to
1a21b77
Compare
setup.cfg
Outdated
| pytest-tinybird>=0.2.0 | ||
| aws-cdk-lib>=2.88.0 | ||
| websocket-client>=1.7.0 | ||
| localstack-snapshot~=0.1.0 |
There was a problem hiding this comment.
Chiming in here from the side, after discussing this PR in localstack/localstack-snapshot#2. 😛
And unfortunately, I cannot comment on line 95, which is why I will just add two comments in one here:
- line # 95: Would be great to change this exact pin to a min version as discussed in remove pin on jsonpath-ng and fix issue localstack-snapshot#2.
- line # 118: With the new dependency pinning we have a coordinated update mechanism, which automatically tries to update the lock files to the highest possible version. With remove pins in setup.cfg, add .python-version #10195 we removed as many pins as possible to increase the efficiency of the automated updates. I think we should now avoid upper version limits wherever possible. It would be great if we could replace
~with>in this line.
There was a problem hiding this comment.
Regarding the second point, I was mostly trying to avoid > due to the lib not strictly following semantic versioning yet. rolo had the same constraint, but since this was removed in the unpinning PR I guess we can remove it here as well.
dominikschubert
force-pushed
the
snapshot-lib-extraction
branch
from
bc4ec4a to
56addfd
Compare
| jsonpath-ng==1.5.3 | ||
| # via localstack-core (pyproject.toml) | ||
| # via | ||
| # localstack-core (pyproject.toml) | ||
| # localstack-snapshot |
There was a problem hiding this comment.
Interesting observation here. The pin stays the same because of the transitive dependency/constraint from localstack-snapshot 0.1.0, but for non-test targets the dependency is actually updated.
In our current test setup this will mean the localstack we're running our tests against will have a different version of jsonpath-ng than the one that we ship 🤔
I'm not sure this is a situation we want. This would be resolved by testing against the actual built container without mounting/installing any test dependencies in there, but for now it might lead to some fairly confusing situations.
This also prompts the question if we should maybe add a way to highlight/restrict these conflicting cases somewhere (e.g. in the periodic dependency updates), since this can continue occurring for other dependencies as well. cc @alexrashed @silv-io
There was a problem hiding this comment.
Wow, great catch! That is a very good point, and great input for an upcoming iteration on the dependency pinning automation (and maybe the pre-commit hook)!
For this particular case, we should then move forward, merge the upgrade in localstack-snapshot, create a release, and fix the issue hat hand (different versions for testing and publishing).
5 participants
Motivation
In our ongoing effort to externalize core components of our development framework, the snapshot testing lib is now next on the line.
Changes
localstack-snapshotlib (naming will probably change later, just a generic name for now)is_aws()replaced withis_aws_cloud()fromlocalstack.testing.aws.utilFollow-ups / TODOs
There's still a bit of refactoring left, but the lib can at least now be used without calling AWS like it previously did. It also doesn't add any default transformers on its own, so that's why we need to add them in our conftest ourselves now.
The transformer utility file hasn't been moved since most of the transformers in there don't make sense in a generic context. Will iterate over this at a later point.