Skip to content

Conversation

@marcelmamula
Copy link
Contributor

@marcelmamula marcelmamula commented Jan 3, 2025

Changes

  • Add new variables sap_vm_provision_group_* for sap_vm_provision to customize group names
  • Add new variables sap_vm_temp_vip_group_* for sap_vm_temp_vip to customize group names
  • Update all group[''] conditionals and loops to use new variables

NOTE 1: This does not require any change in AP4S as it adds way to customize provisioning, while retaining previous functionality.

@sean-freeman NOTE 2: Terraform tf_template_* files were not changed, because their hardcoded groups are commented out and unused. If it was need to use in future, we would need to switch to jinja template to fill in variable input into terraform files instead of direct copy.

Test results

All changes were tested with SLES4SAP 15 SP6 on following platforms:

  • AWS
  • AZ
  • GCP

Tested scenarios:

  • SAP HANA HA
  • SAP ASCS/ERS HA

@marcelmamula marcelmamula added the enhancement New feature or request label Jan 3, 2025
@marcelmamula marcelmamula self-assigned this Jan 3, 2025
@ja9fuchs
Copy link
Contributor

ja9fuchs commented Jan 7, 2025

Review ongoing...

@ja9fuchs
Copy link
Contributor

ja9fuchs commented Jan 7, 2025

@marcelmamula Due to the direct relationship of the groups to the sap_host_type host specification dict value, can you confirm that this type is intended to change to the custom group name?
Otherwise many conditionals that match the group name in this parameter would not work, if I am not mistaken. Did you test with the existing default sap_host_type values and custom group names and it worked?

Copy link
Contributor

@ja9fuchs ja9fuchs left a comment

Choose a reason for hiding this comment

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

The role of sap_host_type has been clarified.

The change looks good to me, it only replaces hard-coded names with variables.

@marcelmamula
Copy link
Contributor Author

@marcelmamula Due to the direct relationship of the groups to the sap_host_type host specification dict value, can you confirm that this type is intended to change to the custom group name? Otherwise many conditionals that match the group name in this parameter would not work, if I am not mistaken. Did you test with the existing default sap_host_type values and custom group names and it worked?

@ja9fuchs Providing my explanation to keep this in reply as well.
You are correct and I reflected it in readme as well.
previously: sap_host_type is one out of hana_primary, hana_secondary, nwas_ascs, nwas_ers, nwas_pas, nwas_aas, anydb_primary, anydb_secondary

now: sap_host_type can be any value that you define in corresponding sap_vm_provision_group_* variables, which be picked during workflow and compared against sap_host_type

sap_host_type is required, because you use that dictionary to define final system layout and if you did not have it, then groups would never be established to drive further logic that decides what goes where

@sean-freeman
Copy link
Member

Will not test, trust the team 👍

All the same, for Ansible > Terraform I confirm this change will not impact bottom Terraform level logic, as it is just a passthrough of the data from the upper Ansible level:
https://github.com/sap-linuxlab/community.sap_infrastructure/blob/main/roles/sap_vm_provision/tasks/platform_ansible_to_terraform/aws_ec2_vs/tf_template/tf_template_outputs.tf#L8

Copy link
Member

@sean-freeman sean-freeman left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@sean-freeman sean-freeman merged commit ce5ab91 into sap-linuxlab:dev Feb 14, 2025
3 of 4 checks passed
@marcelmamula marcelmamula deleted the groups branch July 15, 2025 11:34
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.

3 participants