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

Add module #1

Merged
merged 35 commits into from
Nov 12, 2024
Merged

Add module #1

merged 35 commits into from
Nov 12, 2024

Conversation

Phil-Thoennissen
Copy link
Collaborator

Initital Module.

Please take a closer look at INSTALL.md line 52-60

Copy link
Member

@rswrz rswrz left a comment

Choose a reason for hiding this comment

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

Also do:

  • Add pre-commits (see terraform-vm-azurerm)

versions.tf Outdated Show resolved Hide resolved
versions.tf Outdated Show resolved Hide resolved
variables.tf Show resolved Hide resolved
tests/main.tftest.hcl Outdated Show resolved Hide resolved
stlaunchpadprd.tf Outdated Show resolved Hide resolved
examples/usage/main.tf Show resolved Hide resolved
kvlaunchpadprd.tf Outdated Show resolved Hide resolved
stlaunchpadprd.tf Outdated Show resolved Hide resolved
vnet-launchpad-prd.tf Outdated Show resolved Hide resolved
id-launchpad-prd.tf Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@rswrz rswrz left a comment

Choose a reason for hiding this comment

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

Remote test missing

Phil-Thoennissen and others added 15 commits September 17, 2024 15:03
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: Roman Schwarz <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: Roman Schwarz <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
@rswrz rswrz changed the title initial commit with the module Add module Sep 17, 2024
Copy link
Member

@rswrz rswrz left a comment

Choose a reason for hiding this comment

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

Thank you for developing this module! I've added some comments and recommendations in my review. Please take these as constructive feedback aimed at improving the module. They are based on the Terraform Module Style Guide, which is currently under development.

If you'd like, we can discuss the individual points in a meeting.

  • Please remove the empty main.tf file after splitting resources into multiple files.
  • Ensure all references to renamed resources and data sources are updated accordingly.
  • Rename the tests/local/mock_providers directory to tests/local/mocks for consistency.
  • Please also use the latest module-ci.yaml workflow. This workflow also runs all tests (example, local, remote), and this would fail in this PR.

I did not review the INSTALL.md for now. I will do this as soon as the module is working.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
stlaunchpadprd.tf Outdated Show resolved Hide resolved
examples/usage/main.tf Show resolved Hide resolved
tests/remote/main.tftest.hcl Outdated Show resolved Hide resolved
tests/remote/main.tftest.hcl Show resolved Hide resolved
@rswrz rswrz self-requested a review October 8, 2024 08:28
INSTALL.md Outdated Show resolved Hide resolved
data.tf Outdated Show resolved Hide resolved
tests/local/main.tftest.hcl Outdated Show resolved Hide resolved
tests/local/main.tftest.hcl Outdated Show resolved Hide resolved
tests/local/main.tftest.hcl Outdated Show resolved Hide resolved
tests/local/main.tftest.hcl Outdated Show resolved Hide resolved
tests/remote/resources.tf Outdated Show resolved Hide resolved
tests/remote/terraform.tf Outdated Show resolved Hide resolved
tests/remote/terraform.tf Outdated Show resolved Hide resolved
tests/remote/terraform.tf Outdated Show resolved Hide resolved
Quick Fix for several suggestions

Co-authored-by: Andre Licht <[email protected]>
Signed-off-by: Phil-Thoennissen <[email protected]>
.gitignore Show resolved Hide resolved
Copy link
Member

@rswrz rswrz left a comment

Choose a reason for hiding this comment

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

The module is really coming together. Thanks for your work on this! 👍🏻

Here are a few small things that still need to be adjusted.

examples/usage/README.md Outdated Show resolved Hide resolved
examples/usage/tests_override.tf Outdated Show resolved Hide resolved
examples/usage/README.md Outdated Show resolved Hide resolved
examples/usage/main.tf Outdated Show resolved Hide resolved
tests/remote/resources.tf Outdated Show resolved Hide resolved
r-network.tf Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
@rswrz
Copy link
Member

rswrz commented Oct 11, 2024

As a side note, the remote test failed.

lixhunter
lixhunter previously approved these changes Oct 11, 2024
Signed-off-by: philthoennissen <[email protected]>
Phil-Thoennissen and others added 4 commits October 15, 2024 14:47
Co-authored-by: Roman Schwarz <[email protected]>
Signed-off-by: Phil-Thoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
@Phil-Thoennissen
Copy link
Collaborator Author

@rswrz Everything should be done now and pipelines are green as well.

@rswrz
Copy link
Member

rswrz commented Oct 23, 2024

Hey @Phil-Thoennissen,

I’ve noticed that the Storage Account (also in the upstream Launchpad Terraform code) allows Blob Public Access by default. We should change this. Please set allow_nested_items_to_be_public to false.

Copy link
Member

@rswrz rswrz left a comment

Choose a reason for hiding this comment

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

Thank you for your work! There are still a few minor issues with the documentation and the primary usage example. Please don't hesitate to reach out if you need any clarification.

examples/usage/main.tf Show resolved Hide resolved
examples/usage/main.tf Outdated Show resolved Hide resolved
examples/usage/main.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
@Phil-Thoennissen Phil-Thoennissen merged commit 7ea14ec into main Nov 12, 2024
11 checks passed
@Phil-Thoennissen Phil-Thoennissen deleted the add_module branch November 12, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants