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

[Feature] Added automated provisioning of CosmosDB and App Insights for OPEA applications - Infosys #657

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

kkrishTa
Copy link
Contributor

Description

Infosys contribution to OPEA.
Added CosmosDB and App Insights to the terraform on Azure.

Issues

NA

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)

Dependencies

None

Tests

Tested terraforms on Azure platform.

@lianhao lianhao requested review from poussa and removed request for daisy-ycguo December 25, 2024 07:04
Copy link
Collaborator

@mkbhanda mkbhanda left a comment

Choose a reason for hiding this comment

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

a few questions

indexing_mode = "consistent"

included_path {
path = "/*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does not /* cover everything? Is there some ordering effect here .. from most general to specific?

name = "workspace-${var.cluster_name}"
location = azurerm_resource_group.main.location
resource_group_name = azurerm_resource_group.main.name
sku = "PerGB2018"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really want to hard code sku or keep it a variable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sku is not what one might think. PerGB2018 is default, so we should leave it out for simplicity. If needed, we can add it later via variable.

@poussa
Copy link
Collaborator

poussa commented Jan 7, 2025

@kkrishTa thanks for the PR. I think this is good addition. Few questions below.

Can we make CosmosDB optional (opt-in)? User should have an option to include CosmosDB or leave it out.

Do we have any OPEA apps using CosmosDB?

@poussa
Copy link
Collaborator

poussa commented Jan 15, 2025

@kkrishTa can you adress my questions above. I think we can merge this.

Copy link
Collaborator

@poussa poussa left a comment

Choose a reason for hiding this comment

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

Can we make CosmosDB optional (opt-in)? User should have an option to include CosmosDB or leave it out.

Do we have any OPEA apps using CosmosDB?

@kkrishTa
Copy link
Contributor Author

Can we make CosmosDB optional (opt-in)? User should have an option to include CosmosDB or leave it out.

Do we have any OPEA apps using CosmosDB?

Yes, We can make CosmosDB optional. Will update the code shortly. Apologies for late reply.

@kkrishTa kkrishTa requested review from poussa and mkbhanda January 20, 2025 06:28
@@ -101,3 +101,8 @@ variable "throughput" {
error_message = "Cosmos db throughput should be in increments of 100."
}
}

variable "is_cosmosdb_required" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add default (false) so use is not asked every time?

Copy link
Contributor Author

@kkrishTa kkrishTa Jan 22, 2025

Choose a reason for hiding this comment

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

Added and added one more parameter in tfvars also defaulted to false. Mentioned in README that users can update this if they want Cosmos DB as part of their provisioning.

@poussa
Copy link
Collaborator

poussa commented Jan 22, 2025

LGTM

@lianhao
Copy link
Collaborator

lianhao commented Jan 22, 2025

Just FYI. v1.2rc branch is created minutes ago. If this PR is still targeted for v1.2, please set the milestone to v1.2, so Suyue can cherry-pick it to v1.2 rc branch later.

@kkrishTa
Copy link
Contributor Author

Just FYI. v1.2rc branch is created minutes ago. If this PR is still targeted for v1.2, please set the milestone to v1.2, so Suyue can cherry-pick it to v1.2 rc branch later.

@poussa Will you be able to do this?

@poussa poussa added this to the v1.2 milestone Jan 22, 2025
@mkbhanda mkbhanda added the v1.2 label Jan 22, 2025
Copy link
Collaborator

@mkbhanda mkbhanda left a comment

Choose a reason for hiding this comment

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

LGTM

@mkbhanda mkbhanda merged commit d29bd2d into opea-project:main Jan 22, 2025
7 checks passed
chensuyue pushed a commit that referenced this pull request Jan 24, 2025
…or OPEA applications - Infosys (#657)

* Updated terraform files with CosmosDB and App Insights

Signed-off-by: kkrishTa <[email protected]>

* Updating code with correct names

Signed-off-by: kkrishTa <[email protected]>

* correcting terraform

Signed-off-by: kkrishTa <[email protected]>

* Added opt in variable for cosmosdb provisioning

Signed-off-by: kkrishTa <[email protected]>

* Defaulting Cosmos DB provisioning to false and added variable in tfvars

Signed-off-by: kkrishTa <[email protected]>

---------

Signed-off-by: kkrishTa <[email protected]>
(cherry picked from commit d29bd2d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants