Add possible disable created service and propose new approach about customize ignore_changes

[ByJacob]

what

• Add possible disable created service

why

Sometimes there is a need to add ignore_changes to sites. The current approach is to copy the entire block. I propose to output the necessary variables to the output, and then add the appropriate block that will create the service with your code.

references

Don't create issue :)

[Gowiem]

Adding @nitrocode as I seem him as the ECS expert 😄

[nitrocode]

I'm not sure if I completely understand this PR.

It seems like there is an addition of 2 things.

1. disable the module
2. output all the parameters for the ecs service

disable the module

The ability to disable the service can be done using var.enabled

module "ecs_alb_service_task" {
  source = "cloudposse/ecs-alb-service-task/aws"
  # Cloud Posse recommends pinning every module to a specific version
  # version = "x.x.x"

  enabled = false

  # etc
}

The other way to do this in more recent versions of terraform is using count

module "ecs_alb_service_task" {
  source = "cloudposse/ecs-alb-service-task/aws"
  # Cloud Posse recommends pinning every module to a specific version
  # version = "x.x.x"

  count = 0

  # etc
}

output all the parameters for the ecs service

Not sure how this is valueable. We already copy and paste the input arguments for the ecs service 4 times and this new output will be adding a 5th...

I'm unsure what problem this is trying to solve.

[mergify[bot]]

This pull request is now in conflict. Could you fix it @ByJacob? 🙏

[ByJacob]

I'm not sure if I completely understand this PR.

It seems like there is an addition of 2 things.

1. disable the module
2. output all the parameters for the ecs service

disable the module

The ability to disable the service can be done using var.enabled

module "ecs_alb_service_task" {
  source = "cloudposse/ecs-alb-service-task/aws"
  # Cloud Posse recommends pinning every module to a specific version
  # version = "x.x.x"

  enabled = false

  # etc
}

The other way to do this in more recent versions of terraform is using count

module "ecs_alb_service_task" {
  source = "cloudposse/ecs-alb-service-task/aws"
  # Cloud Posse recommends pinning every module to a specific version
  # version = "x.x.x"

  count = 0

  # etc
}

output all the parameters for the ecs service

Not sure how this is valueable. We already copy and paste the input arguments for the ecs service 4 times and this new output will be adding a 5th...

I'm unsure what problem this is trying to solve.

Regarding the first one, I would like to add the ability to disable only aws_ecs_service. Currently, this is not possible.

Regarding the second thing: I want to add the possibility that everyone in their scope can configure lifecycle -> ignore_changes. Currently, the entire block is copied to ignore one parameter, and I assume there will be many more use cases.

[nitrocode]

Oh I see. You want to use all the business logic within the module but none of the ecs services itself because you'd like to add your own custom lifecycle ignore rules. So you output all the business logic, create nothing, and input that logic into your own custom aws_ecs_service resource.

In your PR, most of the outputted information you propose are already module inputs... Instead of outputting all of the information, couldn't you simply just disable the aws_ecs_service creation with a new flag and create your own aws_ecs_service using the module outputs that already exist ?

If you don't create the ecs service, you're basically just creating the iam role and the task definition. You might as well create a custom module.

[ByJacob]

Oh I see. You want to use all the business logic within the module but none of the ecs services itself because you'd like to add your own custom lifecycle ignore rules. So you output all the business logic, create nothing, and input that logic into your own custom aws_ecs_service resource.

In your PR, most of the outputted information you propose are already module inputs... Instead of outputting all of the information, couldn't you simply just disable the aws_ecs_service creation with a new flag and create your own aws_ecs_service using the module outputs that already exist ?

If you don't create the ecs service, you're basically just creating the iam role and the task definition. You might as well create a custom module.

That's exactly what I meant. However, I wanted all the values to be in one variable. Additionally, some things are not derived:

• aws_ecs_service name
• task_definition - it consists of several variables
• launch_type, platform_version, network_configuration and scheduling_strategy depend on the conditions

If it was a problem to derive this variable, I would like to add only the option to disable the creation of the ecs website.

[nitrocode]

You could do the following to keep it DRY

• Create a local that contains all of the ecs task def inputs
• Update the single output to use the local
• Reuse the local in each ecs task definition input

We have seen issues in the past with outputting too much information as it can be blocked if passing in sensitive information thus failing the instantiation of the module... so instead of outputting everything in a single output, it would be better to output only what you need (as you described above).

By the way, what is the additional lifecycle hook that you're trying to add?

[mergify[bot]]

This pull request is now in conflict. Could you fix it @ByJacob? 🙏

[ByJacob]

By the way, what is the additional lifecycle hook that you're trying to add?

I created Blue/Green deploy using terraform and must ignore changes for task_definition and network_configuration.

You could do the following to keep it DRY

• Create a local that contains all of the ecs task def inputs
• Update the single output to use the local
• Reuse the local in each ecs task definition input

• create local variable ecs_service_attrs
• update output
• updated the attributes in 4 aws_ecs_service blocks

[Nuru]

I am skeptical of the premise of this PR. We do not, in general, want to be creating modules that do nothing but generate outputs, but you may have a valid exception here. At the moment I do not have time to give it the thought it deserves.

Nevertheless, I am marking this as "do not merge" as we are in a code freeze on this and many other modules until we release a final version of terraform-aws-security-group and associated standards and best practices.

[ByJacob]

I am skeptical of the premise of this PR. We do not, in general, want to be creating modules that do nothing but generate outputs, but you may have a valid exception here. At the moment I do not have time to give it the thought it deserves.

I do not agree with you. This module, after disabled aws_ecs_service, does the following:

• aws_ecs_task_definition
• "aws_iam_role" "ecs_task"
• "aws_iam_role_policy_attachment" "ecs_task"
• "aws_iam_role" "ecs_service"
• "aws_iam_role_policy" "ecs_service"
• "aws_iam_role_policy" "ecs_ssm_exec"
• "aws_iam_role" "ecs_exec"
• "aws_iam_role_policy" "ecs_exec"
• module "security_group"

I disabled only resource "aws_ecs_service" and add output

[ByJacob]

@Nuru Do you know when it will be possible to continue this MR?

[nitrocode]

The more I think about it, the more outputting that information to construct your own aws_ecs_service outside of this module, seems like the more scalable solution since lifecycle arguments cannot use variables.

Otherwise we'll have more PRs like this https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/127 to add every iteration of a lifecycle argument.

[nitrocode]

@Nuru as a follow up to my last comment. Now I'm in need of this 😄

There are times when you want to create task/exec roles, task definition but without the aws_ecs_service. For example, when implementing a task that's run once i.e. runs using a aws ecs run-task command which does not require an aws_ecs_service but does require the other mentioned resources.

As an additional benefit of this PR, the output that creates the aws_ecs_service can then be used to construct your own service with your own lifecycle ignore rules without having to create every permutation as a separate resource.

I'm now a proponent of this PR.

Edit: Blocked by https://github.com/cloudposse/terraform-aws-security-group/pull/17

[ByJacob]

@nitrocode https://github.com/cloudposse/terraform-aws-security-group/pull/17 are merged :tada:

What would I have to do now?

[nitrocode]

@ByJacob the next step is to create a separate pr to

• use the new version of the security group module similar to the way it was used before the pre-releases
  • I don't believe we have an example for this yet (cc: @Nuru)
  • This will probably add/subtract module inputs for the sg module
• create migration steps for the before pre-releases versions (tf migrations, changes in inputs, etc)

I believe this is already in the works so probably best to wait. If you're feeling ambitions, feel free to start and tag @Nuru to make sure it's setup the way it was intended.

Thanks again for your contribution. I assure you your contribution is helpful and will eventually be merged in.

[ByJacob]

@Nuru @nitrocode Sorry for the long response time.

• Add extra PR, where will update sg versions first?
• Somewhere I can find a sample PR where SG is raised to version 0.4?
• By migration steps, I mean a step-by-step list of what needs to be done in this module to keep it working as it works? Or is there an example already?

I think I am able to undertake a sg update.

[mergify[bot]]

This pull request is now in conflict. Could you fix it @ByJacob? 🙏

[nitrocode]

@ByJacob it was opted to revert the sg module in this repo so now it's back to using the raw resources which means the repo is now unfrozen. Apologies for the delay.

[ByJacob]

@nitrocode I'm back to this module and now can finish this MR :D

[ByJacob]

@goruha Can you look for this PR ? :)

[hans-d]

@ByJacob hi, can you update this pr so that all tests pass? otherwise this PR will be closed due to staleness. thanks!

[mergify[bot]]

This pull request is now in conflict. Could you fix it @ByJacob? 🙏

[hans-d]

😿 not helping...

  variables.tf:499:1: warning: variable "service_created" is declared but not used ()
  time=2024-03-07T10:39:30.356Z level=INFO msg="reviewdog: failed to post a review comment: POST https://api.github.com/repos/cloudposse/terraform-aws-ecs-alb-service-task/pulls/119/reviews: 403 Resource not accessible by integration []"
  reviewdog: This GitHub Token doesn't have write permission of Review API [1],
  so reviewdog will report results via logging command [2] and create annotations similar to
  github-pr-check reporter as a fallback.

will do some internal poking...

[ByJacob]

😿 not helping...

  variables.tf:499:1: warning: variable "service_created" is declared but not used ()
  time=2024-03-07T10:39:30.356Z level=INFO msg="reviewdog: failed to post a review comment: POST https://api.github.com/repos/cloudposse/terraform-aws-ecs-alb-service-task/pulls/119/reviews: 403 Resource not accessible by integration []"
  reviewdog: This GitHub Token doesn't have write permission of Review API [1],
  so reviewdog will report results via logging command [2] and create annotations similar to
  github-pr-check reporter as a fallback.

will do some internal poking...

I check it in evening

[hans-d]

Can you check this bit of the failing check? variables.tf:499:1: warning: variable "service_created" is declared but not used ()

[mergify[bot]]

Thanks @ByJacob for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

[!TIP]

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

[mergify[bot]]

[!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.

[mergify[bot]]

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

[mergify[bot]]

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