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

aws-ec2: Instance.applyCloudFormationInit is private #30863

Closed
Irame opened this issue Jul 15, 2024 · 6 comments · Fixed by #30907
Closed

aws-ec2: Instance.applyCloudFormationInit is private #30863

Irame opened this issue Jul 15, 2024 · 6 comments · Fixed by #30907
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@Irame
Copy link

Irame commented Jul 15, 2024

Describe the bug

  1. Can not attach CloudFormationInit to an Instance after I created it because Instance.applyCloudFormationInit is not public.

Expected Behavior

I expected to be able to call applyCloudFormationInit to attach my CloudFormationInit to a EC2 Instance, because the documentation said it would be possible.

Current Behavior

I can not call Instance.applyCloudFormationInit beacuse it it private.

Reproduction Steps

Create a ec2.Instance and try to call applyCloudFormationInit on it.

Possible Solution

Make ec2.Instance.applyCloudFormationInit public.

Additional Information/Context

Code location:

private applyCloudFormationInit(init: CloudFormationInit, options: ApplyCloudFormationInitOptions = {}) {

Documentation: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ec2.CloudFormationInit.html#:~:text=As%20an%20app%20builder%2C%20use%20instance.applyCloudFormationInit()

CDK CLI Version

2.149.0

Framework Version

No response

Node.js Version

v20.11.0

OS

Windows

Language

TypeScript

Language Version

5.5.2

Other information

No response

@Irame Irame added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 15, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Jul 15, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Jul 15, 2024
@khushail khushail self-assigned this Jul 15, 2024
@khushail
Copy link
Contributor

Hi @Irame , thanks for reaching out. AFAIK, the applyCloydformationInit() has been inherently called by EC2 Instance object creation constructor as mentioned here which makes call to attach() to set the config.

* Use a CloudFormation Init configuration at instance startup

Making it public might not be desirable PR(014c13a#diff-53d5a35c408bb5bcd8c3b8e135dbd00660447763e9003af414536ab603d4a25d). Leaving it upto the core team for decision making.

@khushail khushail added p2 effort/small Small work item – less than a day of effort p1 needs-review and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 labels Jul 15, 2024
@Irame
Copy link
Author

Irame commented Jul 15, 2024

Hi, thank you for the prompt response.
I noticed that applyCloydformationInit() is called by the constructor, but I didn't see any reason for it to be private. Making it public would be beneficial because reimplementing its functionality using attach is cumbersome and error-prone. Additionally, applyCloydformationInit() includes a call to waitForResourceSignal(), which is also private and similarly difficult to reimplement.

@khushail khushail removed their assignment Jul 16, 2024
@khushail
Copy link
Contributor

Hi @Irame , I checked with my team regarding this method. Seems like the change was done quite a long time ago. I also saw that this method is public with autoscaling -

public applyCloudFormationInit(init: ec2.CloudFormationInit, options: ApplyCloudFormationInitOptions = {}) {

It would make no harm if we make the mentioned method public for EC2 as well. Please feel free to submit a PR if you would like. Thanks!

Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

obraafo pushed a commit to obraafo/aws-cdk that referenced this issue Jul 25, 2024
### Issue # (if applicable)

Closes aws#30863.

### Reason for this change

The issue is asking to make the function `applyCloudFormationInit()` public from private.

Also we have this function being public in https://github.com/aws/aws-cdk/blob/ffd9d9ce510a4c820b1437cce93c4772cd7c14c0/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts#L1650 

### Description of changes

Make the function public

### Description of how you validated changes
build passed

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants