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

* Fixed errors and warning messages about ELB Logs S3 bucket #243

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

aleksandarknezevic
Copy link
Contributor

@aleksandarknezevic aleksandarknezevic commented Jun 25, 2023

what

  • Introduces using cloudposse s3-bucket module for creating ALB logging bucket
  • Introducing new variable which defines if s3 logs for ALB is enabled or disabled (default is still true)
  • Adding random suffix to the name of logging bucket (since names of S3 must be globally unique - very often name provided in module is in collision with some already existed)
  • If created, S3 bucket for storing ALB access logs is encrypted by default
  • Updated examples (modules vpc, subnet and alb were in old versions)

why

references

@aknysh
Copy link
Member

aknysh commented Jun 28, 2023

@aleksandarknezevic thanks for the PR.
Please review and fix the status checks above (terraform fmt the module and example, version pinning, etc.)
Then run the following commands locally and commit the changes:

make init
make github/init
make readme

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

@aleksandarknezevic
Copy link
Contributor Author

aleksandarknezevic commented Jun 28, 2023

@aknysh Thank you for suggestions. I solved mentioned problems. You can see my local run here. Second validate fails due to missing ACCESS_TOKEN or APP_ID which is expected on my github. Could we re-run it please?

main.tf Outdated Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@aleksandarknezevic thanks for the updates, looks good

Please see the comment

@aknysh
Copy link
Member

aknysh commented Jun 29, 2023

@aleksandarknezevic thanks again for all the work on the PR.
Please fix https://github.com/cloudposse/terraform-aws-elastic-beanstalk-environment/actions/runs/5417366681/jobs/9848261046?pr=243 by running

make init
make github/init
make readme

@aleksandarknezevic
Copy link
Contributor Author

@aknysh Fixed and squashed. Here is run on my env. Could you please re-run it?

@aknysh
Copy link
Member

aknysh commented Jun 29, 2023

/terratest

@aknysh
Copy link
Member

aknysh commented Jun 29, 2023

/terratest

@aleksandarknezevic
Copy link
Contributor Author

aleksandarknezevic commented Jun 29, 2023

It looks like terratest run using terraform 0.15, we would need at least terraform 1.3.0 here.

+ [[ -n main ]]
+ VERSION=0.15
+ echo Full version to use is 0.15.5, setting VERSION to 0.15
Full version to use is 0.15.5, setting VERSION to 0.15

Is it possible to force terratest to use another version of terraform?

@aknysh
Copy link
Member

aknysh commented Jun 29, 2023

added the terraform-1.x

added the terraform-1.x label

@aleksandarknezevic
Copy link
Contributor Author

added the terraform-1.x

added the terraform-1.x label

Not sure if label is enough. Last run used terraform 0.15 as well.

@aknysh
Copy link
Member

aknysh commented Jun 29, 2023

@aleksandarknezevic
Copy link
Contributor Author

aleksandarknezevic commented Jun 30, 2023

@aknysh Thank you. Done. Could you please re-run actions?

* ELB Log bucket is switched to submodule cloudposse/s3-bucket/aws
* Added option to chose enabling ALB logs
* Updated examples (bumped versions for modules vpc, subnet and alb)
@aknysh
Copy link
Member

aknysh commented Jun 30, 2023

/terratest

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@aknysh aknysh merged commit 92c3ba4 into cloudposse:main Jun 30, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants