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

detect cloud aws workaround #87

Merged
merged 2 commits into from
Mar 18, 2022

Conversation

yeoldegrove
Copy link
Collaborator

No description provided.

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.

I would put this abstraction in the module detect_cloud function rather than here

@@ -596,6 +596,16 @@ def cloud_grains_present(
__salt__['grains.set']('cloud_provider', cloud_provider)
changes['cloud_provider'] = cloud_provider

# workaround for https://github.com/SUSE/ha-sap-terraform-deployments/issues/832
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say this piece of code should go in the crm.detect_cloud module, and not here.
In fact, I don't know why we have the gcp part there as well.
This means that the detect_cloud module function is kind of broken otherwise

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The gcp part does not change the cloud_provider but additional grains gcp_instance_id/name.
I guess this is fine...I guess there is just no better place to set this at the moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arbulu89 Force pushed an update which moves this to the module.

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.

Thanks!

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.

2 participants