-
Notifications
You must be signed in to change notification settings - Fork 39
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
Improvements and bug fixes #47
base: main
Are you sure you want to change the base?
Improvements and bug fixes #47
Conversation
… internal sql columns
c90765f
to
d74fd81
Compare
) | ||
|
||
var ( | ||
dinosaurTableName = util.ToSnakeCase(api.DinosaurTypeName) + "s" | ||
dinosaurColumns = []string{ |
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 means to support searching a new column, it must be added to this list?
I see clusters-service implementing this requirement in this way, but AMS/OSDFM seem to handle this differently.
I'm not sure which is better tbh, the AMS implementation is explicit while this implementation is simpler. @markturansky can you provide your input here?
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, I'd like to eventually be able to move away from this to have something that uses reflect or similar to have it be more dynamic
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'd prefer to not explicitly set casing, as that negates the casing config allowed by gorm (that also does things like table name prefix, etc). can we do this through gorm somehow,?
What
Tests
Unit
Integration