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

module.codebuild.local_file.php_ini can cause unnecessary TF changes #64

Open
nvnivs opened this issue May 4, 2022 · 2 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@nvnivs
Copy link
Collaborator

nvnivs commented May 4, 2022

One issue with the module.codebuild.local_file.php_ini resource, whenever Terraform executes on a new machine it will trigger a replacement as the file does not exist.

It's a known thing as per https://registry.terraform.io/providers/hashicorp/local/latest/docs/resources/file

Note about resource behaviour
When working with local files, Terraform will detect the resource as having been deleted each time a configuration is applied on a new machine where the file is not present and will generate a diff to re-create it. This may cause "noise" in diffs in environments where configurations are routinely applied by many different users or within automation systems.

When using ephemeral machines to run terraform this causes the terraform to detect this as a change every time. In one side is a nuisance to have TF reporting changes when they actully there are none.

The biggest issue however is that when this resource changes, it triggers the following resource updates:

  • module.codebuild.local_file.php_ini
  • module.codebuild.data.archive_file.code_build_package
  • module.codebuild.aws_s3_object.wordpress_dockerbuild
  • null_resource.trigger_build

So images are being build and stored in ECR with no changes, which is an unecessary cost.

2 things come to my mind that we could consider:

  • An alternative implementation to the local_file resource
  • A mechanism to define retention for ECR images
@petewilcock
Copy link
Contributor

petewilcock commented May 4, 2022

Happy to consider a replacement for local_file. We would need another way to set the same values.

Some of this is controlled by the behaviour of the docker image for Wordpress. We're already using a customised entrypoint script to do some ECS-specific things (like get the public IP address and update the Route53 DNS).

One of the features I merged in recently was the ability to set WP_MEMORY_LIMIT, which is something that can't be set in PHP.ini, and due to various quirks of how the docker image works, the image doesn't respect certain environment variables in the config file if the file already exists (i.e. on a second launch of an existing installation).

So my eventual solution was to use the Wordpress CLI in the entrypoint to set the value passed through from the module: https://github.com/TechToSpeech/terraform-aws-serverless-static-wordpress/blob/release/0.2.0/modules/codebuild/codebuild_files/docker-entrypoint.sh#L322

Perhaps a similar thing could be done for these values currently configured in PHP.ini... would need some investigation.

@nvnivs
Copy link
Collaborator Author

nvnivs commented May 6, 2022

Thanks for the feedback @petewilcock!

Im neither a PHP or Wordpress expert, but after a bit of Googling around it does look like the WP CLI supports those settings.

Will also have a look at implemention an aws_ecr_lifecycle_policy. Regarless of this issue, having a mechanism to cleanup ECR images would be a nice to have.

I will assign this one to myself if you don't mind. Doesn't sound like others are getting very troubled by this 😄

@nvnivs nvnivs self-assigned this May 6, 2022
@nvnivs nvnivs added the enhancement New feature or request label May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants