search

theforeman / foreman_azure_rm

Adds Azure Resource Manager as a compute resource for The Foreman
GNU General Public License v3.0
9 stars 24 forks source link

Fixes #29222 - Dynamically load regions based on subscription #79

Closed apuntamb closed 4 years ago

apuntamb commented 4 years ago

build failures are related, will fix the tests.

ShimShtein commented 4 years ago

What about GovClouds? Are those shown by this call?

apuntamb commented 4 years ago

What about GovClouds? Are those shown by this call?

That's what we are currently unaware of. Trying to check with someone who can confirm this with Govcloud credentials.

apuntamb commented 4 years ago

What about GovClouds? Are those shown by this call?

As per our discussion, we can specifically keep this PR for generic regions and once this will be in, we can take care of gov regions separately as a different PR.

vijay8451 commented 4 years ago

ACK from me, @vijay8451, your turn :)

sure , will give a test run :)

vijay8451 commented 4 years ago

Test Results:

Good:

Requires below are handle as well:

azurerm_conn ec2_test_conne

kgaikwad commented 4 years ago
  • When press the button Load Regions the AzureRm shows regions list along with error can't be blank and requires to click on Test Connection to check the connection however with EC2 it does not shows the same but also do auto check the Test Connection as well

IIRC, EC2 don't have any validation on region field that's why it is not showing validation error on test connection.

vijay8451 commented 4 years ago
  • When press the button Load Regions the AzureRm shows regions list along with error can't be blank and requires to click on Test Connection to check the connection however with EC2 it does not shows the same but also do auto check the Test Connection as well

IIRC, EC2 don't have any validation on region field that's why it is not showing validation error on test connection.

Yeah, you are right it is the one reason here , however I have following opinion on it:

apuntamb commented 4 years ago
  • When press the button Load Regions the AzureRm shows regions list along with error can't be blank and requires to click on Test Connection to check the connection however with EC2 it does not shows the same but also do auto check the Test Connection as well

IIRC, EC2 don't have any validation on region field that's why it is not showing validation error on test connection.

Yeah, you are right it is the one reason here , however I have following opinion on it:

* After press the button `Load Regions` it should not show the message `can't be blank` because we are trying to fill that place not submitting CR create without it .

So, as @kgaikwad mentioned, it's true that EC2 doesn't have any validation for region field hence, even if you create an EC2 CR without region, it's created.

* Message `can't be blank` should show if anyone is trying to `Submit` the CR create request without region then it would be more appropriate.

Region will be made optional for creating a CR similar to EC2, so that it won't show can't be blank message.

* In `EC2` after click on `Load Regions` button it does auto check the `Test Connection` as well however with `AzureRm` still a manual check after regions get loaded.

For this and the other fields to show can't be blank instead of Internal Server Error 500 on Test Connection in case of empty input fields, I am creating a separate PR for the same and will reference it here.

apuntamb commented 4 years ago

@vijay8451 @ShimShtein PR #84 handles the test connection method to show errors on empty input fields.

vijay8451 commented 4 years ago

@vijay8451 @ShimShtein PR #84 handles the test connection method to show errors on empty input fields.

Sounds good to me , ACK :+1:

ShimShtein commented 4 years ago

Merged, thanks @apuntamb!