Skip to content
This repository has been archived by the owner on Feb 14, 2024. It is now read-only.

feat(module/bootstrap)!: Enhanced user experience of the module usage #166

Merged
merged 18 commits into from
Aug 12, 2022

Conversation

FoSix
Copy link
Contributor

@FoSix FoSix commented Jun 30, 2022

Description

Small refactor of the bootstrap module that follows requirements found in real-life.
Things worth mentioning are:

  • unify the storage account name variable - use the same one regardless if the storage is being created or only sourced.
  • add File Share quota variable - this still default to 50G but now can be customized
  • add File Share access tier variable - for the 3.x provider setting this to null causes a share to be recreated during each Terraform run. Defaults to Cool for cost limitation (it is only used once, during bootstrap) but can be customised.

The bootstrap example was also modified to show how the module can be used to create a single Storage Account with two File Shares for Transit VNET Ref Arch (based on real requirements).

Motivation and Context

  • unify the storage account name variable - having two variables for storage name was:
    • counter-schema when looking at other modules
    • confusing - we have a variable that controls the module behaviour, existing_storage_account was duplicating the logic
  • add File Share access tier variable - this was causing issues when updating the infrastructure

How Has This Been Tested?

The boostrap example was used to test the module development. All examples using the module were run to verify backwards compatibility. If braking changes were found, examples were adjusted accordingly.

Screenshots (if appropriate)

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

@FoSix FoSix marked this pull request as ready for review June 30, 2022 13:55
@FoSix FoSix requested a review from migara June 30, 2022 13:56
@FoSix FoSix self-assigned this Jun 30, 2022
@FoSix FoSix linked an issue Jun 30, 2022 that may be closed by this pull request
Copy link
Member

@migara migara left a comment

Choose a reason for hiding this comment

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

This modules needed this refactor, great job 🙌🏽

modules/bootstrap/variables.tf Outdated Show resolved Hide resolved
modules/bootstrap/variables.tf Outdated Show resolved Hide resolved
modules/bootstrap/variables.tf Outdated Show resolved Hide resolved
modules/bootstrap/variables.tf Show resolved Hide resolved
@FoSix FoSix requested a review from michalbil July 1, 2022 06:16
@migara migara changed the title !feat(module/bootstrap): clean up variables feat(module/bootstrap)!: clean up variables Jul 1, 2022
@migara migara changed the title feat(module/bootstrap)!: clean up variables feat(module/bootstrap)!: Enhanced user experience of the module usage Jul 1, 2022
@FoSix FoSix requested a review from migara July 1, 2022 11:07
examples/bootstrap/README.md Outdated Show resolved Hide resolved
examples/bootstrap/example.tfvars Outdated Show resolved Hide resolved
examples/transit_vnet_dedicated/main.tf Outdated Show resolved Hide resolved
examples/panorama/variables.tf Outdated Show resolved Hide resolved
examples/panorama/example.tfvars Outdated Show resolved Hide resolved
modules/bootstrap/variables.tf Show resolved Hide resolved
examples/transit_vnet_dedicated/main.tf Show resolved Hide resolved
examples/bootstrap/main.tf Show resolved Hide resolved
Copy link
Member

@migara migara left a comment

Choose a reason for hiding this comment

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

LGMT 👏🏽

examples/bootstrap/outputs.tf Outdated Show resolved Hide resolved
examples/bootstrap/outputs.tf Outdated Show resolved Hide resolved
FoSix and others added 2 commits July 8, 2022 11:32
Copy link
Contributor

@michalbil michalbil left a comment

Choose a reason for hiding this comment

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

LGTM! with one last suggestion :)

examples/panorama/example.tfvars Show resolved Hide resolved
Copy link
Member

@migara migara left a comment

Choose a reason for hiding this comment

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

Great work!

@FoSix FoSix merged commit edc34bb into develop Aug 12, 2022
@FoSix FoSix deleted the module_bootstrap_simplify_variables branch August 12, 2022 13:04
@github-actions
Copy link
Contributor

🎉 This PR is included in version 0.4.0 🎉

The release is available on Terraform Registry and GitHub release

Posted by semantic-release bot

sebastianczech pushed a commit that referenced this pull request Oct 18, 2023
…#166)

* refactor of the bootstrap module + example

* adjust the existing module invocaton to reflect changes to variables

* following sugestions from Migara

* readme update

* adjusting panorama example to fit changes made to bootstrap module

* Update examples/bootstrap/README.md

Co-authored-by: michalbil <[email protected]>

* Update examples/transit_vnet_dedicated/main.tf

Co-authored-by: michalbil <[email protected]>

* Update examples/panorama/example.tfvars

Co-authored-by: michalbil <[email protected]>

* pr suggestions from @michalbil

* adding validation for the storage_share_name variable

* updates to readmes

* Update examples/bootstrap/outputs.tf

Co-authored-by: Migara Ekanayake <[email protected]>

* Update examples/bootstrap/outputs.tf

Co-authored-by: Migara Ekanayake <[email protected]>

* adding back missing panorama name property

* update readme

* upgrading minimum version of terraform due to usage of alltrue function

* update readme's

Co-authored-by: michalbil <[email protected]>
Co-authored-by: Migara Ekanayake <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

module/bootstrap: clean up variables
4 participants