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

sap_*_preconfigure: Tests with ansible parameter -i instead of -l #671 #672

Closed
wants to merge 2 commits into from

Conversation

streitd
Copy link

@streitd streitd commented Mar 5, 2024

This PR is changing ansible-playbook parameter -l to -i in tests scripts.
No more need to specify hosts in the file /etc/ansible/hosts each time.

@berndfinger berndfinger self-requested a review March 7, 2024 13:33
@berndfinger
Copy link
Member

Hi @streitd,
We should also add -u root to the ansible-playbook command (so that a normal user can also run the test).
We also should fix the same issue for the role sap_hana_install .
Feel free to use the code from berndfinger@8c40e91 and add it as a new commit to your branch so that it will be part of this PR.

@streitd
Copy link
Author

streitd commented Mar 7, 2024

@berndfinger Your code has been merged

Copy link
Member

@berndfinger berndfinger left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@berndfinger berndfinger left a comment

Choose a reason for hiding this comment

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

Wait! We need to merge into dev, not in main.

@berndfinger berndfinger changed the base branch from main to dev March 7, 2024 15:37
@berndfinger berndfinger changed the base branch from dev to main March 7, 2024 15:38
Copy link
Member

@berndfinger berndfinger left a comment

Choose a reason for hiding this comment

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

I tried to modify the destination branch to main but this would lead to lots of other changes to be reverted.
@streitd Can you please create a new PR with all the changes but based on the upstream dev tree?

@berndfinger
Copy link
Member

I just created #680. Will close this PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants