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

feat: moderinze setup to use labels #16

Closed
wants to merge 18 commits into from

Conversation

esolitos
Copy link

@esolitos esolitos commented Apr 23, 2024

what

Modernizes the module setup

  • Added the use of cloudposse/label/null for standardized naming
  • Ran make actions to sync, as recommended in another PR.
  • Updated examples/complete to a better standard

⚠️ Changes are not backward compatible, resources will be recreated and their name will change.

why

  • elevate the module to the same standard as the other cloudposse modules 😃

references

closes: #3

@mergify mergify bot added the triage Needs triage label Apr 23, 2024
make readme
make github/init
@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label Apr 23, 2024
Copy link

mergify bot commented Apr 23, 2024

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@esolitos esolitos marked this pull request as ready for review April 23, 2024 13:31
@esolitos esolitos requested review from a team as code owners April 23, 2024 13:31
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@esolitos couple requests, but generally this is looking good. This will need an elevated approval, but I'll give my approval once changes are made and then ask the CP team to give it a review as well. Thanks!

alarms.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@Gowiem
Copy link
Member

Gowiem commented Apr 24, 2024

/terratest

@esolitos
Copy link
Author

I believe I fixed the failures reported by the CI. :)

@Gowiem
Copy link
Member

Gowiem commented Apr 24, 2024

/terratest

@esolitos
Copy link
Author

🙃 Or not.
I did the mistake of running the make script as first step, let me fix that.

@Gowiem
Copy link
Member

Gowiem commented Apr 24, 2024

/terratest

@esolitos
Copy link
Author

Not sure what is failing.

Is it because the example is in ./examples/simple instead of ./examples/complete?

If yes, is renaming this in scope for this PR? 😅 I have no problem doing it, takes me 1 minute, I'm jsut wondering.

@Gowiem
Copy link
Member

Gowiem commented Apr 24, 2024

@esolitos yeah it looks like this module is so old it wasn't following where we put our primary example path (which is ancient) and therefore our newer CI pipes are failing as they expect that path. I'd move simple to complete and let's give it another run 👍

@Gowiem
Copy link
Member

Gowiem commented Apr 24, 2024

/terratest

@Gowiem
Copy link
Member

Gowiem commented Apr 24, 2024

Ah I think we need a versions.tf file for examples/complete like we have here: https://github.com/cloudposse/terraform-aws-transit-gateway/blob/main/examples/complete/versions.tf

It may be that we need more than that too... We'll see what the test harness does. This is an old module that hasn't seen love in a long while... so you picked a good one 😁

@esolitos
Copy link
Author

I did update the examples/complete with some additional love'n'care. 😄

@Gowiem
Copy link
Member

Gowiem commented Apr 25, 2024

/terratest

@Gowiem
Copy link
Member

Gowiem commented Apr 25, 2024

Haha I didn't realize there isn't even a test/src directory... @esolitos if we want this to pass our standard tests we would need to go all the way towards adding Terratest to this repo like in https://github.com/cloudposse/terraform-aws-transit-gateway/tree/main/test OR if I should just pull in Cloud Posse folks and see if they can get around to reviving this module from what you've already done.

Copy link

mergify bot commented May 3, 2024

💥 This pull request now has conflicts. Could you fix it @esolitos? 🙏

@mergify mergify bot added the conflict This PR has conflicts label May 3, 2024
@mergify mergify bot removed the conflict This PR has conflicts label May 6, 2024
@Gowiem
Copy link
Member

Gowiem commented May 6, 2024

We still don't have terratests on this repo, so no use in trying to run them. I may take a crack at adding them at some point this week or next if I have the time. Thanks for the patience @esolitos!

Copy link

mergify bot commented May 21, 2024

💥 This pull request now has conflicts. Could you fix it @esolitos? 🙏

@mergify mergify bot added the conflict This PR has conflicts label May 21, 2024
@mergify mergify bot closed this May 28, 2024
Copy link

mergify bot commented May 28, 2024

This PR was closed due to inactivity and merge conflicts. 😭
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot removed conflict This PR has conflicts needs-cloudposse Needs Cloud Posse assistance triage Needs triage labels May 28, 2024
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.

Use Labels
2 participants