-
Notifications
You must be signed in to change notification settings - Fork 11
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
Override not editable fields during updates #200
Override not editable fields during updates #200
Conversation
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.
Great catch! Thank you so much
As discussed offline, if there is a quick way via introspection/parsing to determine the // annotation
so to make sure is updated with the list of fields using pointer semantic in the struct, it would be awesome.
If that takes too much time, could be postponed for later
+1 I will try to investigate if there is an easy way to ensure that all fields that are editable are NOT overridden, keep you posted here. |
I think I have found a possible approach for testing this using reflection and inspection of the goverter methods directives. Hope to provide a proposal on Monday :) |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
==========================================
- Coverage 74.33% 72.53% -1.81%
==========================================
Files 15 16 +1
Lines 2159 2210 +51
Branches 68 68
==========================================
- Hits 1605 1603 -2
- Misses 405 452 +47
- Partials 149 155 +6 ☔ View full report in Codecov by Sentry. |
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.
Awesome! I understand works as expected, and the parser used is even in Go stdlib
func checkEntity(entity *oapiEntity) []string { | ||
res := []string{} | ||
objType := reflect.TypeOf(entity.obj) | ||
for i := 0; i < objType.NumField(); i++ { |
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 suspect there must be a way to make Field(s) iterable with range
but it's not important for now
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.
TBH I couldn't find a different way to iterate over those fields, at least using the reflect
pkg and types. But yeah we can figure out if we can improve it in a later iteration
entityName := v.extractGroup(converterMethodPattern, methodName) | ||
entity := v.getEntity(entityName) | ||
// there should be just one doc comment matching ignoreDirectivePattern | ||
for _, c := range m.Doc.List { |
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 understand this extract all code comments pertaining to Methods. Nice, just as expected also from JavaParser AST I believe structure is analogous
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.
yeah correct, it extracts all of them and then I just had to filter the one we are insterested in.
Going to merge this to unblock the development of opendatahub-io/model-registry#173 |
…endatahub-io#200) Bumps [ruff](https://github.com/astral-sh/ruff) from 0.5.2 to 0.5.4. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.5.2...0.5.4) --- updated-dependencies: - dependency-name: ruff dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes https://github.com/opendatahub-io/model-registry/issues/199
Description
As per related issue description, this PR aims to fix the update issue for
InferenceService
andServeModel
.Created
OverrideNotEditableFor<Entity>
converters that take<existing, update>
and override the NOT editable fields in theupdate
object by taking those values from theexisting
.With this approach I was able to remove the
Name
override in the core functions.NOTE: everytime we add a new field in the openapi spec that can be editable, we should
ignore
it in theOverrideNotEditableFor<Entity>
converters.How Has This Been Tested?
Merge criteria: