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

if exec is enabled, also enable init process #189

Closed
wants to merge 14 commits into from

Conversation

kevcube
Copy link
Contributor

@kevcube kevcube commented Mar 29, 2022

Seems PR template is ignored if I create PR from GitHub.dev

what

why

  • it will remove zombie processes in containers after exec is run
  • because AWS recommended it

(if initProcessEnabled is already defined, var.linux_parameters.initProcessEnabled will remain.)
@kevcube kevcube requested review from a team as code owners March 29, 2022 07:56
@kevcube kevcube requested review from a team as code owners March 29, 2022 07:57
@kevcube kevcube changed the title if exec is enabled, also enable init process(if initProcessEnabled is already defined, var.linux_parameters.initProcessEnabled will remain.) if exec is enabled, also enable init process Mar 29, 2022
@kevcube kevcube marked this pull request as draft March 29, 2022 08:40
@kevcube
Copy link
Contributor Author

kevcube commented Mar 29, 2022

Tough to implement because both results of a conditional must be the same type, but container_definition module expects either the defined object as input variable, or null

Will be easier when defaults are released.

@mihaiplesa
Copy link
Contributor

Would this work now?

@Gowiem
Copy link
Member

Gowiem commented Nov 20, 2022

@kevcube any movement on this one? Worth continuing or should we close it out?

@kevcube kevcube marked this pull request as ready for review December 3, 2022 13:51
@kevcube
Copy link
Contributor Author

kevcube commented Dec 3, 2022

@Gowiem this is ready for review

I based the optional/required state of parameters on this page https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-tmpfs.html

This also raises minimum terraform version to 1.3.0, not sure if we're ok with this (cc @nitrocode)

Copy link
Member

@nitrocode nitrocode left a comment

Choose a reason for hiding this comment

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

See comments

@kevcube kevcube requested a review from nitrocode April 18, 2023 22:16
@kevcube
Copy link
Contributor Author

kevcube commented Apr 18, 2023

@nitrocode now that a decision has been made regarding updating terraform, can this be merged?

@nitrocode nitrocode dismissed their stale review April 18, 2023 22:25

deferring to admins

@Gowiem
Copy link
Member

Gowiem commented Apr 19, 2023

@kevcube can you run make init && make github/init locally, commit, and push? That'll fix the auto-format failure.

@Gowiem
Copy link
Member

Gowiem commented Apr 19, 2023

/test all

Gowiem
Gowiem previously approved these changes Apr 19, 2023
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

LGTM, but we still need admins to weigh in as nitro mentioned. I'll share with the rest of the maintainers and we'll get it merged soon 👍

@Gowiem
Copy link
Member

Gowiem commented Apr 19, 2023

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 19, 2023

@kevcube there is a map() function call somewhere in the code. Mind hunting around for it so we can do the upgrade to 1.0?

@Gowiem
Copy link
Member

Gowiem commented Apr 19, 2023

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 19, 2023

@kevcube test failure 😔

TestExamplesComplete 2023-04-19T05:05:31Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: error creating S3 bucket ACL for eg-test-ecs-web-app-complete-333-alb-access-logs: AccessControlListNotSupported: The bucket does not allow ACLs
│ 	status code: 400, request id: 4KT6HD601B48BXYT, host id: 2z9g185BrZVK7LO2iQ4mkSgHyjXk2PCt3WxotqB5pX1f6qXdMGjpSA6bzZ+dFGZXaYvYfP0CKM8=
│ 
│   with module.alb.module.access_logs.module.s3_bucket.module.aws_s3_bucket.aws_s3_bucket_acl.default[0],
│   on .terraform/modules/alb.access_logs.s3_bucket.aws_s3_bucket/main.tf line 148, in resource "aws_s3_bucket_acl" "default":
│  148: resource "aws_s3_bucket_acl" "default" {
│ 
╵}
    apply.go:15: 
        	Error Trace:	apply.go:15
        	            				examples_complete_test.go:40
        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; ╷

@kevcube
Copy link
Contributor Author

kevcube commented Apr 19, 2023

@Gowiem yeah... I had this same error locally but it went away on second apply. It might be a race condition that requires a sleep, but it's deep in the module.alb.module.access_logs... might be due to amazon's recent global changes to S3 permissions

@aknysh
Copy link
Member

aknysh commented Apr 19, 2023

/test terratest

@kevcube
Copy link
Contributor Author

kevcube commented Apr 26, 2023

Seems breakage can be traced to here cloudposse/terraform-aws-s3-bucket#174

After that is fixed, we will need to update terraform-aws-lb-s3-bucket, terraform-aws-alb, then finally update the test.

@hans-d hans-d added the stale This PR has gone stale label Mar 2, 2024
@hans-d hans-d closed this Mar 2, 2024
@mschfh
Copy link
Contributor

mschfh commented Aug 1, 2024

Could this please be reopened/rebased?
Else I'm happy to pick up linux_parameters support in a separate PR

@aknysh
Copy link
Member

aknysh commented Aug 8, 2024

linux_parameters support in a separate PR

@mschfh please add linux_parameters support in a separate PR.
This PR is very old and introduced many changes.

thank you

@mihaiplesa
Copy link
Contributor

@aknysh I've recreated at #285

@mihaiplesa mihaiplesa mentioned this pull request Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This PR has gone stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants