Skip to content
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

add susChkSrv HA/DR provider and make providers configurable #911

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

yeoldegrove
Copy link
Collaborator

  • add susChkSrv HA/DR provider
  • make providers configurable

This will be a part of fixing #910

Needs SUSE/saphanabootstrap-formula#153 to be merged.

@yeoldegrove
Copy link
Collaborator Author

requirements.yml will be bumped as soon as new saphanabootstrap-formula rpm is build.

@yeoldegrove yeoldegrove added the enhancement New feature or request label Nov 7, 2023
aws/variables.tf Outdated Show resolved Hide resolved
@yeoldegrove
Copy link
Collaborator Author

@stefanotorresi @angelabriel Is it fine to enable both hooks here by default?

Copy link
Collaborator

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @yeoldegrove ,
All good codewise. I'm wondering if we should put them disabled by default, as we did in the formula.
No hard feelings.

aws/variables.tf Outdated
variable "hana_ha_dr_sustkover_enabled" {
description = "enable susTkOver hook"
type = bool
default = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value should be false, right?
Or do we want to have it enabled by default.
Coming to the discussion in the formula, as it is not mandatory, we could set it disabled by default.
If you change this, remember to change the tfvars example file

@@ -62,6 +62,9 @@ hana:
monitoring_enabled: {{ grains['monitoring_enabled']|default(False) }}
ha_enabled: {{ grains['ha_enabled'] }}
basepath_shared: {{ grains['hana_basepath_shared']|default(True) }}
ha_dr_sustkover_enabled: {{ grains['hana_ha_dr_sustkover_enabled']|default(True) }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same with the default value

@angelabriel
Copy link
Contributor

@stefanotorresi @angelabriel Is it fine to enable both hooks here by default?

I would prefer to have it consistent, which means disabled by default. For the not mandatory Hooks the customer should actively enable them to get them work. He should be aware of these Hooks and their behavior as they might interfere with other tools he is using for daily work.

@yeoldegrove yeoldegrove force-pushed the feat/susChrSrv branch 2 times, most recently from 53d8438 to 65bb751 Compare November 7, 2023 13:35
@yeoldegrove yeoldegrove removed the request for review from stefanotorresi November 7, 2023 13:55
@yeoldegrove yeoldegrove merged commit d27d45e into main Nov 8, 2023
8 checks passed
@yeoldegrove yeoldegrove deleted the feat/susChrSrv branch November 8, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants