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
Feature/1693 api v3 schema etc #2052
Feature/1693 api v3 schema etc #2052
Changes from 45 commits
f1798b2
0f62851
c01cfda
4fd08e2
5c8e005
458bfed
a7a6cf9
af489b1
21f8008
6f63117
76e87ce
d62cd90
fc4c359
e6c8d8c
3566df5
219360a
112ac1e
101b37a
b934958
51ad013
ba5edce
662119b
b0fd955
d480445
bffbf55
3542a43
51d68a8
e1c904e
489ae12
d6ef401
a75ab85
6a16696
65b9f21
4426ff7
fe6246c
9241eb1
373bf1d
73bb8fd
a2c5f40
4d9f623
09c326e
5df526d
8baf2d3
1c241d1
bf15425
7fe2a93
56a9f0c
a4aac70
5d24819
7040694
e0fda73
2bfd751
1da8e8c
9c3f663
a80d9ac
f851bce
562da60
fb0dd93
56ba9ed
619da37
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Sorry for my ignorance. Why we need dedicated endpoint for mapping table defaults? Isn't it enough to be part of the mapping table entity?
Maybe more a question to @Zejnilovic, as I see it in the proposal doc too.
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.
For GET - because spark-jobs pull that data on the run of the conformance rule, in the middle of the job.
For POST - so you do not have to always update the whole thing.
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.
Yes, the proposal contained it.
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.
GET - 👍
PUT - Why is this different from other partial properties? (Minor, can stay 😉 )
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.
Sort of explained in person, the gist is: a special approach is warranted due to the fact that defaults are added in now. But true that the whole endpoint may not be strictly necessary for V3 API.
Since it was designed like this, I'd like to keep it, but removal should be quite easy. 😏
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.
Why the admin authorization requirement (multiple places)?
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 is a mistake I plan on fixing in #2055, because there are other changes to the admin authorization that need to be done (In short: I have moved the admin-auth to VersionedModelController, but it wrong as only the PropDefs should be admin-controlled)I have fixed this in this PR already, there will be just touchups in #2055.
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.
Seems like the same code as above?
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.
While not completely the same, there are not a lot of differences, I admit.
It did not seem like something worth generalizing to me (do you think 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.
🤷 Up to you, sorry I missed the difference. On the other hand it suggests similarity. Counter argument is, if the code can be logically distilled not just some forced sub-function.
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.
Ok, I have actually created a subfunction that seems to make sense. Hope you find it a tad better, @benedeki.
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.
Appreciate the comment 😎
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.
Glad you liked it, but I actually had to redo it