-
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
Refactoring migration assessment codepath #2137
Conversation
- goal is to eventually get rid of using issue reason for lookups
a35b70f
to
a869cda
Compare
…n cmd for fetching issues
…in AssessmentReport struct
@@ -100,7 +100,8 @@ type DBObject struct { | |||
} | |||
|
|||
// TODO: support MinimumVersionsFixedIn in xml | |||
type Issue struct { | |||
type AnalyzeSchemaIssue struct { |
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 believe you have just changed the struct name, not the JSON field names, but have we tested YugabyteD to confirm if this change does not break anything?
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.
its the struct name only, not the fields. So json will be same.
We send payload(json) to ybd
MigrationCaveats []UnsupportedFeature `json:"MigrationCaveats"` | ||
} | ||
|
||
type AssessmentIssue struct { |
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.
@sanyamsinghal can you add comments to each field, giving an example of what they might look like?
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.
Category - Feature
, Migration Caveat
. Query Constructs
, Datatype
TypeName - Gin Indexes
, Regex Functions
, ...
TypeDescription - TODO: add if available or add for required ones
Impact: Last meeting we discuss it as Level-1, Level-2, Level-3... But i am thinking we can have it as Minor
Moderate
Significant
also
ObjectType - it can be empty in some cases, for categories like unsupported datatypes thinking of the object type as datatype for eg: geometry
Basically thinking of the datatype as object type
We can discuss this more in next PR, for now i just defined a struct. @makalaaneesh
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.
got it, yeah just wanted you to add comments to code next to the field 👍
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.
sure, will add.
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.
@makalaaneesh let me know what do you think about
ObjectType - it can be empty in some cases, for categories like unsupported datatypes thinking of the object type as datatype for eg: geometry
Basically thinking of the datatype as object type
Describe the changes in this pull request
Just minor refactoring to restart with, please review commit by commit to get an idea of what all changes are done.
Describe if there are any user-facing changes
NA
How was this pull request tested?
Not required, existing is enough.
Does your PR have changes that can cause upgrade issues?