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/18743 fix terraform modules warnings AB#18743 #385

Closed
wants to merge 8 commits into from

Conversation

zjanura
Copy link
Contributor

@zjanura zjanura commented Jan 26, 2024

@zjanura zjanura requested review from tom-reinders and a team as code owners January 26, 2024 13:20
@tom-reinders
Copy link
Member

I will take a closer look at this next week.
The problem with this PR is that this would be a breaking change.
We could just accept that and have to update all the lockfiles of existing projects.
An other possible solution could be changing the version constraint to not allow a later version then 2.43.0, and update the hashicorp/azuread version in the next major version we are doing.

@zjanura
Copy link
Contributor Author

zjanura commented Jan 29, 2024

Sure, take your time and lets decide how do we approach to this.

@@ -47,7 +47,7 @@ resource "azurerm_api_management_api" "api" {
authorization_server_name = "${lower(replace(var.api_settings.name, " ", "-"))}-auth"
}

soap_pass_through = var.soap_pass_through
api_type = var.api_type
Copy link
Contributor

@ArtiomMatiom ArtiomMatiom Jan 29, 2024

Choose a reason for hiding this comment

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

indeed this is a breaking change (anyone who uses this module, it will stop working.

Could you keep the variable named:
soap_pass_through but make it something as follow:

api = var. api_type == null && var.soap_pass_through? "soap": var.api_type !=null? var. api_type : "http"

This way: even if someone updates the repo, it will still work with the same interface ( input variables)

variable "api_type" {
type = string
description = "Type of API. Possible values are graphql, http, soap, and websocket. Defaults to http"
default = "http"
Copy link
Contributor

Choose a reason for hiding this comment

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

based on my suggestion to not break other peoples code, I would put to null as default

@tom-reinders
Copy link
Member

tom-reinders commented Jan 29, 2024

That alone is not the only breaking change.

Updating azuread_application.application.application_id to azuread_application.application.id will also break stuff.

For this to work we need to change the version of

    azuread = {
      source  = "hashicorp/azuread"
      version = "~> 2.36"
    }

application_id will only work in if we change it to version = ">= 2.36, < 2.44", because it was removed in 2.44.0, doing would in most cases not break our code, and only break for projects that have updated there .terraform.lock.hcl files to a version after 2.43.0.

For versions 2.44.0 and 2.45.0 nether will work.

id will only work in if we change it to version = "~> 2.46", because it was added in 2.46.0, doing this would be a breaking change as updating the version constraint would couse us to update all existing .terraform.lock.hcl files in all our projects.

@zjanura zjanura changed the base branch from develop to 4.x February 2, 2024 14:27
@zjanura zjanura changed the base branch from 4.x to develop February 15, 2024 11:05
@zjanura zjanura closed this Feb 19, 2024
@tom-reinders tom-reinders deleted the feature/18743-Fix-terraform-modules-warnings branch February 19, 2024 10:39
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.

4 participants