-
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
fix: only add PSC ipType if PSC is enabled #388
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.
Add new testcases for the new logic. Don't replace the existing testcases in test/sqladmin-fetcher-ts
.
I'm curious about why you decided to inline the logic from parseIpAddresses()
into the SqlAdminFetcher
and get rid of the unit tests.
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 CAS-based instances being supported soon and also using
dnsName
,we can no longer rely on the presence of
dnsName
in the connectSettingsAPI as identifying that PSC is enabled on an instance.
Instead we should check
pscEnabled
from the response as source of truth.Check if
pscEnabled
isTrue
before setting PSC ip type.Moving
parseIPAddresses
into private method onSQLAdminFetcher