Skip to content
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 content-ID to REST API v2 entries endpoint #5879

Merged
merged 2 commits into from Jan 17, 2023

Conversation

snazy
Copy link
Member

@snazy snazy commented Jan 17, 2023

Fixes #5878

@snazy snazy requested a review from dimas-b January 17, 2023 16:21
Copy link
Member

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix. LGTM overall. Minor comments.

.as(EntriesResponse.class);
assertThat(entries.getEntries())
.hasSize(1)
.allSatisfy(e -> assertThat(e.getContentId()).isNull());
Copy link
Member

@dimas-b dimas-b Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think a plain JSON string test like testCommitMetaAttributes for the new content ID attribute is worth adding (in addition to this test, or replacing this test)? My thinking is that java code blurs the case of having a null attribute with not having an attribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to leave it out.

@snazy
Copy link
Member Author

snazy commented Jan 17, 2023

Hum - something's broken :(

@snazy
Copy link
Member Author

snazy commented Jan 17, 2023

Damn - if namespaceDepth > 0 is specified, the "flattening" doesn't work ATM.

@snazy
Copy link
Member Author

snazy commented Jan 17, 2023

Damn - if namespaceDepth > 0 is specified, the "flattening" doesn't work ATM.

Ah - not an issue at all - that's only used from the namespace API, which doesn't get the content-ID anyway - so all good :)

@dimas-b
Copy link
Member

dimas-b commented Jan 17, 2023

if namespaceDepth > 0 is specified [...]

namespaceDepth is not supported in API v2... Does this help?

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 83.90% // Head: 83.92% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (c919845) compared to base (69cc884).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5879      +/-   ##
==========================================
+ Coverage   83.90%   83.92%   +0.02%     
==========================================
  Files          31       31              
  Lines        1491     1493       +2     
  Branches      239      240       +1     
==========================================
+ Hits         1251     1253       +2     
  Misses        172      172              
  Partials       68       68              
Flag Coverage Δ
java ∅ <ø> (∅)
javascript ∅ <ø> (∅)
python 83.92% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/pynessie/client/nessie_client.py 79.87% <0.00%> (+0.26%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@snazy snazy merged commit f779ce0 into projectnessie:main Jan 17, 2023
@snazy snazy deleted the v2-entries-cid branch January 17, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add content ID to EntriesResponse in API v2
2 participants