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.

Sign up for GitHub

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

Jump to bottom

Fix how sidadm user is transformed to lowercase #65

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Fix how sidadm user is transformed to lowercase #65

merged 1 commit into from
Mar 15, 2021

Conversation

arbulu89
Copy link
Collaborator

@arbulu89 arbulu89 commented Mar 12, 2021 •
edited
Loading

The sidadm transformation is implemented incorrectly.

>>> HANAUSER = '{sid}adm'.lower()
>>> HANAUSER.format(sid='PRD')
'PRDadm'
>>>

This should return prdadm.
I have reimplemented the usage to transform correctly to lowercase.
This was affecting the saphanabootstrap-formula usage in the ha_cluster.sls state if the configured sid is in upper case.

We have not found this on the ha-sap-terraform-deployments project because the system identifier is always lowercased before adding to the hana.sls pillar file

FYI: @petersatsuse

@arbulu89 arbulu89 mentioned this pull request Mar 12, 2021
Merged
Simranpal
Simranpal approved these changes Mar 12, 2021
View reviewed changes
Copy link

@Simranpal Simranpal left a comment

Choose a reason for hiding this comment

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

thanks for the fix

shaptools/hana.py
@@ -84,8 +84,6 @@ class HanaInstance(object):
'x86_64', 'ppc64le'
]
SUPPORTED_SYSTEMS = ['Linux']
# SID is usualy written uppercased, but the OS user is always created lower case.
HANAUSER = '{sid}adm'.lower()
Copy link

Choose a reason for hiding this comment

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

We are already setting the user to lowercase here. I guess this wasnt working in python? that it shows PRDadm

Copy link
Collaborator Author

@arbulu89 arbulu89 Mar 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah... This stupid piece of code doesn't work XD The lower function is applied before the formatting.
And we didn't have any proper UT neither

@arbulu89 arbulu89 merged commit 75f5f46 into SUSE:master Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Simranpal Simranpal Simranpal approved these changes

@stefanotorresi stefanotorresi Awaiting requested review from stefanotorresi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arbulu89 @Simranpal