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

Update ID lists to be empty #163

Merged
merged 1 commit into from
Jun 23, 2024
Merged

Conversation

gregops312
Copy link
Contributor

@gregops312 gregops312 commented Jun 20, 2024

When using modules/cloudwatch-logs the module "lambda" could have constant churn because of the possible introduction of "" in the list of vpc_subnet_ids and vpc_security_group_ids

Description

When using modules/cloudwatch-logs the module "lambda" could have constant churn because of the possible introduction of "" in the list of vpc_subnet_ids and vpc_security_group_ids

How Has This Been Tested?

I used my branch and ran against the churning issue. Screenshots have been added to PR as well.

Before

before

After

after

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)
    • This isn't a readme change, but it should have no negative effects that I can fathom.

@gregops312 gregops312 requested a review from a team as a code owner June 20, 2024 15:17
@gregops312 gregops312 force-pushed the fix/cloudwatch-logs branch 2 times, most recently from 7b065be to c8b4363 Compare June 20, 2024 15:50
When using `modules/cloudwatch-logs` the `module "lambda"` could have constant churn because of the possible introduction of `""` in the list of `vpc_subnet_ids` and `vpc_security_group_ids`

Signed-off-by: Gregory Kman <gregorykman@gmail.com>
@royfur
Copy link
Contributor

royfur commented Jun 20, 2024

Hi @gregops312

Please note that cloudwatch-logs module is deprecated and you should use the coralogix-aws-shipper instead

@gregops312
Copy link
Contributor Author

Hi @gregops312

Please note that cloudwatch-logs module is deprecated and you should use the coralogix-aws-shipper instead

Oh that's good to know. We can work on moving to that soon. So what is the process in getting this PR reviewed and available. Since it still exists I see no reason why it couldn't or shouldn't per say be maintained if people put forth the effort until it is entirely removed.

@royfur
Copy link
Contributor

royfur commented Jun 21, 2024

The reason it is not removed yet is since we didnt want to break customers environment in case they dont use specific version
The deprecation of this module was on May 1st and it is not maintained anymore

one of the reason that it is deprecated is that the lambda node version is not supported anymore in AWS and thats why we moved

@gregops312
Copy link
Contributor Author

The reason it is not removed yet is since we didnt want to break customers environment in case they dont use specific version The deprecation of this module was on May 1st and it is not maintained anymore

one of the reason that it is deprecated is that the lambda node version is not supported anymore in AWS and thats why we moved

It makes sense why it hasn't been removed.

I would like to know what the path forward is on this PR, I see no reason why deprecated changes can't be made to make things better until it is removed. And you yourself made the point that is hasn't been removed because it is in use by customers, like myself, who would like to be able to use it and make it work for me, until at which point I can switch to the new implementation.

@royfur royfur requested review from guyrenny and removed request for a team June 23, 2024 06:58
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.

Looks good

@royfur royfur merged commit 0598741 into coralogix:master Jun 23, 2024
5 checks passed
@gregops312
Copy link
Contributor Author

Looks good

Looks good

Thank you very much! We'll definitely put some work on our backlog for changing to the new module!

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.

3 participants