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/cds 1390 firehose #167

Merged
merged 22 commits into from
Aug 27, 2024
Merged

Feature/cds 1390 firehose #167

merged 22 commits into from
Aug 27, 2024

Conversation

ryantanjunming
Copy link
Contributor

@ryantanjunming ryantanjunming commented Jul 24, 2024

Description

  • Added custom naming for resources
  • Added ability to data reference global resources (s3 & iam)
    🛑 Breaking changes 🛑
    Update variables: private_key renamed to api_key with type string instead of any.

How Has This Been Tested?

Locally

Checklist:

  • I have updated the relevant example in the examples directory for the module.
  • I have updated the relevant test file for the module.
  • This change does not affect module (e.g. it's readme file change)

@ryantanjunming ryantanjunming requested a review from a team as a code owner July 24, 2024 11:15
Copy link
Contributor

@guyrenny guyrenny left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@guyrenny guyrenny left a comment

Choose a reason for hiding this comment

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

Need to also update the variables.tf file in the examples folder.

modules/firehose-metrics/README.md Outdated Show resolved Hide resolved
modules/firehose-metrics/README.md Outdated Show resolved Hide resolved
modules/firehose-metrics/README.md Outdated Show resolved Hide resolved
modules/firehose-metrics/main.tf Outdated Show resolved Hide resolved
modules/firehose-metrics/README.md Outdated Show resolved Hide resolved
modules/firehose-metrics/main.tf Outdated Show resolved Hide resolved
modules/firehose-metrics/output.tf Show resolved Hide resolved
@ryantanjunming ryantanjunming merged commit a35bed5 into master Aug 27, 2024
5 checks passed
Copy link

🎉 This PR is included in version 1.0.105 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@gregops312
Copy link
Contributor

I have to ask why this change was released and you didn't release a major version change, since after all swapping arguments is a breaking change to your consumers. I am specifically referring to moving from private_key to api_keyin firehose-metrics

And the documentation for the module has no note of the change, its only visible in the commit, and never made it in the release notes.

@ryantanjunming
Copy link
Contributor Author

Hi @gregops312, many apologies I should have written it in the PR, it was documented in the ChangeLog.md: https://github.com/coralogix/terraform-coralogix-aws/blob/feature/cds-1390-firehose/CHANGELOG.md#v10105

There will also be another change to the coralogix_region soon to follow https://coralogix.com/docs/user-guides/account-management/account-settings/coralogix-domain/

@gregops312
Copy link
Contributor

Hi @gregops312, many apologies I should have written it in the PR, it was documented in the ChangeLog.md: https://github.com/coralogix/terraform-coralogix-aws/blob/feature/cds-1390-firehose/CHANGELOG.md#v10105

There will also be another change to the coralogix_region soon to follow https://coralogix.com/docs/user-guides/account-management/account-settings/coralogix-domain/

Thanks for that, I stupidly didn't actually look at the ChangeLog.md that is definitely on me. Thanks I will put a note in our usage so we are ready for when that changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants