-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: Update CC to 55 and adopt E2EI changes [WPB-7126] #2691
feat: Update CC to 55 and adopt E2EI changes [WPB-7126] #2691
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.
LGTM, with a question.
value.displayName, | ||
value.domain, | ||
value.certificate, | ||
value.x509Identity?.handle ?: "", |
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.
Question: since the inner object is nullable. Imo we should return null instead of hardcoded values?
value.x509Identity?.handle ?: "", | ||
value.x509Identity?.displayName ?: "", | ||
value.x509Identity?.domain ?: "", | ||
value.x509Identity?.certificate ?: "", |
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.
please do not add default empty strings, if the value is supposed to be nullable now then keep is nullable
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.
change to keep nullable values as it is
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
* feat: Update CC to 55 and adotp E2EI changes * Fix according to the review * CodeStyle fix * Fix tests
What's new in this PR?