-
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
Ensure all the required fields are populated in the html report for new flattened issues approach #2190
Conversation
Jenkins Pipelines |
issue.Reason = strings.Split(issue.Reason, "on column -")[0] | ||
default: | ||
issue.Reason = sensitiveReason | ||
if strings.HasPrefix(issue.Reason, sensitiveReason) { |
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.
pls test this logic manually once with callhome. @priyanshi-yb has a server that you can use to easily test.
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.
verified.
@@ -1235,7 +1237,7 @@ func fetchUnsupportedPlPgSQLObjects(schemaAnalysisReport utils.SchemaReport) []U | |||
}) | |||
} | |||
feature := UnsupportedFeature{ | |||
FeatureName: reason, | |||
FeatureName: issueName, |
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 will have callhome/yugabyted implications, no? Do you want to keep it the same now (set it to reason), and we can discuss this later when dealing with callhome/yugabyted?
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.
The issue with grouping by reason(/description) anymore is that it is dynamic(%s
s), having column info etc..
There will be new unnecessary groups if we go that route for different value of %s in each case.
migtests/tests/pg/omnibus/expected_files/expected_schema_analysis_report.json
Outdated
Show resolved
Hide resolved
migtests/tests/pg/omnibus/expected_files/expected_schema_analysis_report.json
Outdated
Show resolved
Hide resolved
…eason - adding new field 'Name' to analyzeSchemaIssue to store corresponding info from QueryIssue
…ll issue's description/reason
…e to start with 'Unsupported datatype' prefix
…e to start with 'Unsupported datatype' prefix
d6f6608
to
62d82c7
Compare
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.
LGTM
Describe the changes in this pull request
This PR includes:
Describe if there are any user-facing changes
nothing as such, only text has changed for some of the issue fields (reported in assess and analyze)
How was this pull request tested?
Does your PR have changes that can cause upgrade issues?