-
Notifications
You must be signed in to change notification settings - Fork 280
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
Harmonization of resource names for SAP HANA SR clusters #20401
Harmonization of resource names for SAP HANA SR clusters #20401
Conversation
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
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
@@ -129,11 +129,12 @@ sub run_cmd { | |||
|
|||
sub get_promoted_hostname { | |||
my ($self) = @_; | |||
my $master_resource_type = get_var('USE_SAP_HANA_SR_ANGI') ? "mst" : "msl"; |
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 don't see mst_
resources in SUSE/qe-sap-deployment#284. Are they coming later?
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.
Yes, It's preparation for angi code.
@@ -298,11 +299,12 @@ sub is_hana_resource_running { | |||
my ($self, %args) = @_; | |||
$args{quiet} //= 0; | |||
my $hostname = $self->{my_instance}->{instance_id}; | |||
my $master_resource_type = get_var('USE_SAP_HANA_SR_ANGI') ? "mst" : "msl"; |
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 code looks like doing the same of https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/20401/files#diff-4b7291e584e1d0b487f685a7b45efc74dcd804ee2236acf7a866003d0f4fb87aR132
I suggest moving it in a sub
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.
? It's local one line statement. What we will benefit from have it in sub?
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.
to keep this logic centralized, let say tomorrow we want to change setting name and/or triplet to use as name
@@ -208,10 +208,11 @@ sub run { | |||
# Do the HANA "magic" if needed | |||
if ($cluster_type eq 'hana') { | |||
my $remoteHost = $hostname; | |||
my $master_resource_type = get_var('USE_SAP_HANA_SR_ANGI') ? "mst" : "msl"; |
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 can see this file only used in schedule schedule/publiccloud/ha_sap.yml
It this schedule or test module still used anywhere? Is there a way to run a VR for this change? Is this test code related to qe-sap-deployment?
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.
DUNNO. It's another hard coded resource name and now it will work much better, if run.
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
…#20401) * Harmonization of resource names for HANA HA * Update 11_hacluster.t
Harmonization of resource names for SAP HANA SR clusters
Must be merged togather with SUSE/qe-sap-deployment#287
Verification run: