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

refactor!: simplify application stack configuration #207

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sebastianlolv
Copy link
Contributor

@sebastianlolv sebastianlolv commented Aug 7, 2024

Checklist

  • I've updated both the azurerm_linux_web_app and azurerm_windows_web_app resources.

Ref: equinor/terraform-baseline#126

@sebastianlolv sebastianlolv self-assigned this Aug 7, 2024
@sebastianlolv sebastianlolv requested a review from a team as a code owner August 7, 2024 07:32
@hknutsen
Copy link
Member

I'd prefix the PR with refactor!, since it doesn't add or remove anything per se, instead it replaces a complex object variable with multiple simpler variables corresponding to the object properties. See Reviewing changes for reference.

@sebastianlolv sebastianlolv changed the title feat: simplify application stack configurations fo Web Apps refactor!: simplify application stack configurations fo Web Apps Aug 21, 2024
@hknutsen hknutsen changed the title refactor!: simplify application stack configurations fo Web Apps refactor!: simplify application stack configuration Aug 21, 2024
@hknutsen
Copy link
Member

Also removed the for Web Apps part from the PR title, since this change is being made in the Web App module repo 😅

Since this is now marked as a breaking change, it's important that we describe the breaking change in detail in the squash commit message, according to the Conventional Commits specification.

So the squash commit should look something like this:

refactor!: simplify application stack configuration

BREAKING CHANGE: replace complex type variable `application_stack` with simple
type variables `application_stack_docker_image_name`,
`application_stack_docker_registry_url`,
`application_stack_docker_registry_username`,
`application_stack_docker_registry_password` and `current_application_stack`.

^ Make sure it doesn't break the 80 character line limit.

Copy link
Member

@hknutsen hknutsen left a comment

Choose a reason for hiding this comment

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

Added some clarifications to variable descriptions.

Also, with the current implementation, the application_stack block will always be configured, while the previous implementation would only configure it when explicitly specified. We need to test if the current implementation breaks anything, or if we need to re-implement logic to create it dynamically when needed.

variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
sebastianlolv and others added 6 commits August 21, 2024 10:12
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
@hknutsen
Copy link
Member

hknutsen commented Aug 21, 2024

Followup... Module currently throws an error if not setting explicit values for any application stack variables, since it tries to create an empty application_stack block. We'll need to implement logic to only configure the application_stack block when needed.

Copy link
Contributor

There has been no activity on this pull request for 60 days. stale label will be added. If no additional activity occurs, the pull request will be closed in 7 days.

@github-actions github-actions bot added the stale label Oct 21, 2024
Copy link
Contributor

There has been no activity on this stale pull request for 7 days. This pull request will now be closed. If new activity occurs, the pull request will reopen.

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