Обсуждение: Feature #7178 - PostgreSQL deployment on Microsoft Azure

Hi Khushboo

Can you please review it? 

On Wed, Jun 1, 2022 at 10:11 AM Yogesh Mahajan <yogesh.mahajan@enterprisedb.com> wrote:

Hi,

Please find the attached patch which provides functionality to deploy a Postgres cloud instance on Azure Postgresql.

Thanks,

Yogesh Mahajan

EnterpriseDB

--

	Akshay Joshi Principal Software Architect +91 9767888246 www.enterprisedb.com

Hi Yogesh,

Review comments:

• Step 2: The below statements should be in a different line, like
• • "Azure CLI" will use the currently logged in identity through Azure CLI on the local machine.
  • "Interactive Browser" opens a browser to authenticate a user interactively.

• Disable the next button once authentication is complete.

• Cluster name availability call calls the server on every field change

• Availability zone needs description 

• Add High availability option

Code:

• check_cluster_name_availability should be using the GET method instead of Post

• Fix SonarLint issues

• Do we need the cache_persistence_options as it will create the persistent storage which we do not require I guess?

• Why do we need to call _get_azure_credentials on every request? Can't we store it in the session object?

• Use gettext wherever required in the js file

Thanks,

Khushboo

On Wed, 1 Jun 2022, 10:11 Yogesh Mahajan, <yogesh.mahajan@enterprisedb.com> wrote:

Hi,

Please find the attached patch which provides functionality to deploy a Postgres cloud instance on Azure Postgresql.

Thanks,

Yogesh Mahajan

EnterpriseDB

Hi Yogesh,

• Spelling mistake in below error message
  • Name must be more than 3 characters or more & Shoudld not have capital letter

• Cluster name validation comes after all the fields are validated. It should be done while giving input for the cluster name

• Even If the credentials are incorrect, the authenticate button gets disabled. So, I can't update and re-authenticate

Apart from this, it looks good to me.

Thanks,

Khushboo

On Fri, Jun 10, 2022 at 8:14 PM Yogesh Mahajan <yogesh.mahajan@enterprisedb.com> wrote:

Hi,

Please find the updated patch with documentation.

On Mon, Jun 6, 2022 at 1:10 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Yogesh,

Review comments:

• Step 2: The below statements should be in a different line, like
• • "Azure CLI" will use the currently logged in identity through Azure CLI on the local machine.
  • "Interactive Browser" opens a browser to authenticate a user interactively.

Done.

• Disable the next button once authentication is complete.

As discussed disabled Authentication button once authentication is completed.

• Cluster name availability call calls the server on every field change

Done.

• Availability zone needs description 

This is a generic term with cloud hence not added.

• Add High availability option

Done

Code:

• check_cluster_name_availability should be using the GET method instead of Post

Done.

• Fix SonarLint issues

Done.

• Do we need the cache_persistence_options as it will create the persistent storage which we do not require I guess?

Yes, it is required.

• Why do we need to call _get_azure_credentials on every request? Can't we store it in the session object?

Function returns without calling again credentials if an existing client is present.

• Use gettext wherever required in the js file

Done.

Thanks,

Khushboo

Thanks,

Yogesh Mahajan

EnterpriseDB

 

On Wed, 1 Jun 2022, 10:11 Yogesh Mahajan, <yogesh.mahajan@enterprisedb.com> wrote:

Hi,

Please find the attached patch which provides functionality to deploy a Postgres cloud instance on Azure Postgresql.

Thanks,

Yogesh Mahajan

EnterpriseDB

Hello,

Please find the updated patch which validates the field after the blur event on cluster name.

Thanks,

Yogesh Mahajan

EnterpriseDB

On Tue, Jun 14, 2022 at 1:36 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

On Tue, 14 Jun 2022, 13:29 Yogesh Mahajan, <yogesh.mahajan@enterprisedb.com> wrote:

Hi Khushboo,

Thanks for reviewing the patch.

On Tue, Jun 14, 2022 at 11:35 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Yogesh,

• Spelling mistake in below error message
  • Name must be more than 3 characters or more & Shoudld not have capital letter

Done.

• Cluster name validation comes after all the fields are validated. It should be done while giving input for the cluster name

Current react framework validates parent schema fields only after all children schemas. Hence cluster name is validated after all children schema fields.

Then, I think we should fix this.  

• Even If the credentials are incorrect, the authenticate button gets disabled. So, I can't update and re-authenticate

Done.

Apart from this, it looks good to me.

Thanks,

Khushboo

 

Thanks,

Yogesh Mahajan

EnterpriseDB

 

On Fri, Jun 10, 2022 at 8:14 PM Yogesh Mahajan <yogesh.mahajan@enterprisedb.com> wrote:

Hi,

Please find the updated patch with documentation.

On Mon, Jun 6, 2022 at 1:10 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Yogesh,

Review comments:

• Step 2: The below statements should be in a different line, like
• • "Azure CLI" will use the currently logged in identity through Azure CLI on the local machine.
  • "Interactive Browser" opens a browser to authenticate a user interactively.

Done.

• Disable the next button once authentication is complete.

As discussed disabled Authentication button once authentication is completed.

• Cluster name availability call calls the server on every field change

Done.

• Availability zone needs description 

This is a generic term with cloud hence not added.

• Add High availability option

Done

Code:

• check_cluster_name_availability should be using the GET method instead of Post

Done.

• Fix SonarLint issues

Done.

• Do we need the cache_persistence_options as it will create the persistent storage which we do not require I guess?

Yes, it is required.

• Why do we need to call _get_azure_credentials on every request? Can't we store it in the session object?

Function returns without calling again credentials if an existing client is present.

• Use gettext wherever required in the js file

Done.

Thanks,

Khushboo

Thanks,

Yogesh Mahajan

EnterpriseDB

 

On Wed, 1 Jun 2022, 10:11 Yogesh Mahajan, <yogesh.mahajan@enterprisedb.com> wrote:

Hi,

Please find the attached patch which provides functionality to deploy a Postgres cloud instance on Azure Postgresql.

Thanks,

Yogesh Mahajan

EnterpriseDB