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.

Sign up for GitHub

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

Jump to bottom

codeguruprofiler: Invalid profilingGroupName on resource import #22546

Closed
openwebsolns opened this issue Oct 18, 2022 · 3 comments · Fixed by #22554
Closed

codeguruprofiler: Invalid profilingGroupName on resource import #22546

openwebsolns opened this issue Oct 18, 2022 · 3 comments · Fixed by #22554
Assignees
@corymhall
Labels
@aws-cdk/aws-codeguruprofiler Related to Amazon CodeGuru Profiler bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@openwebsolns
Copy link
Contributor

Describe the bug

When importing a ProfilingGroup by either name or ARN, the resulting construct uses the wrong value for profiling group name.

Expected Behavior

import * as codeguru from "aws-cdk-lib/aws-codeguruprofiler";

const pf = codeguru.ProfilingGroup.fromProfilingGroupName(
  scope,
  "ProfilingGroupId",
  "NameOfProfilingGroup"
);

console.log(pf.profilingGroupName); // "NameOfProfilingGroup"

Current Behavior

Output of above is instead the hard-coded value profilingGroup.

Reproduction Steps

// import * as codeguru from "aws-cdk-lib/aws-codeguruprofiler";

const pf = codeguru.ProfilingGroup.fromProfilingGroupName(
  scope,
  "ProfilingGroupId",
  "NameOfProfilingGroup"
);

console.log(pf.profilingGroupName); // "NameOfProfilingGroup"

Possible Solution

Replace resource when importing for resourceName.

Additional Information/Context

No response

CDK CLI Version

2.44.0

Framework Version

No response

Node.js Version

14.17.0

OS

Linux

Language

Typescript

Language Version

4.8.4

Other information

No response
@openwebsolns openwebsolns added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 18, 2022
@github-actions github-actions bot added the @aws-cdk/aws-codeguruprofiler Related to Amazon CodeGuru Profiler label Oct 18, 2022
@mascur mascur added good first issue Related to contributions. See CONTRIBUTING.md p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Oct 19, 2022
@mascur
Copy link
Contributor

mascur commented Oct 19, 2022

Hi @openwebsolns,

Thank you for reporting this issue. I've confirmed that this is an issue and that your proposed solution would likely work. In the meantime I would suggest using fromProfilingGroupArn().

I've marked this issue as a p2. We most likely won't be able to get to this issue. However, if you or another community member would like to submit a PR we would be able to review that. You can refer to the Contribution Guide here to get started.

@openwebsolns
Copy link
Contributor Author

Thanks! I have a PR coming up soon. I don't think fromProfilingGroupArn would work given that one delegates to the other, and the line in question is in that method anyways.

@openwebsolns openwebsolns mentioned this issue Oct 19, 2022
Merged
1 task
@mergify mergify bot closed this as completed in #22554 Oct 19, 2022
mergify bot pushed a commit that referenced this issue Oct 19, 2022
@openwebsolns
fix(codeguruprofiler): incorrect profiling group name is returned whe…
9934619 
…n using importing (#22554)

Closes #22546. Verified via TDD with new unit test. Before the change, the unit test failed, replicating the bug in the issue with output:

```plain
FAIL test/profiling-group.test.js
  profiling group
    ✓ attach read permission to Profiling group via fromProfilingGroupArn (71 ms)
    ✓ attach publish permission to Profiling group via fromProfilingGroupName (27 ms)
    ✕ use name specified via fromProfilingGroupName (8 ms)
    ✓ default profiling group (21 ms)
    ✓ allows setting its ComputePlatform (14 ms)
    ✓ default profiling group without name (13 ms)
    ✓ default profiling group without name when name exceeding limit is generated (18 ms)
    ✓ grant publish permissions profiling group (25 ms)
    ✓ grant read permissions profiling group (25 ms)

  ● profiling group › use specified via name via fromProfilingGroupName

    expect(received).toEqual(expected) // deep equality

    Expected: "MyAwesomeProfilingGroup"
    Received: "profilingGroup"

      174 |
      175 |     const profilingGroup = ProfilingGroup.fromProfilingGroupName(stack, 'MyProfilingGroup', 'MyAwesomeProfilingGroup');
    > 176 |     expect(profilingGroup.profilingGroupName).toEqual('MyAwesomeProfilingGroup');
          |                                               ^
      177 |   });
      178 |
      179 |   test('default profiling group', () => {

      at Object.<anonymous> (test/profiling-group.test.ts:176:47)
```

----

### All Submissions:

* [Yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [No] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [N/A] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

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

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or 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.

madeline-k pushed a commit that referenced this issue Oct 21, 2022
@openwebsolns @madeline-k
fix(codeguruprofiler): incorrect profiling group name is returned whe…
7ad9fca 
…n using importing (#22554)

Closes #22546. Verified via TDD with new unit test. Before the change, the unit test failed, replicating the bug in the issue with output:

```plain
FAIL test/profiling-group.test.js
  profiling group
    ✓ attach read permission to Profiling group via fromProfilingGroupArn (71 ms)
    ✓ attach publish permission to Profiling group via fromProfilingGroupName (27 ms)
    ✕ use name specified via fromProfilingGroupName (8 ms)
    ✓ default profiling group (21 ms)
    ✓ allows setting its ComputePlatform (14 ms)
    ✓ default profiling group without name (13 ms)
    ✓ default profiling group without name when name exceeding limit is generated (18 ms)
    ✓ grant publish permissions profiling group (25 ms)
    ✓ grant read permissions profiling group (25 ms)

  ● profiling group › use specified via name via fromProfilingGroupName

    expect(received).toEqual(expected) // deep equality

    Expected: "MyAwesomeProfilingGroup"
    Received: "profilingGroup"

      174 |
      175 |     const profilingGroup = ProfilingGroup.fromProfilingGroupName(stack, 'MyProfilingGroup', 'MyAwesomeProfilingGroup');
    > 176 |     expect(profilingGroup.profilingGroupName).toEqual('MyAwesomeProfilingGroup');
          |                                               ^
      177 |   });
      178 |
      179 |   test('default profiling group', () => {

      at Object.<anonymous> (test/profiling-group.test.ts:176:47)
```

----

### All Submissions:

* [Yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [No] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [N/A] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mrgrain pushed a commit to mrgrain/aws-cdk that referenced this issue Oct 24, 2022
@openwebsolns @mrgrain
fix(codeguruprofiler): incorrect profiling group name is returned whe…
98a4115 
…n using importing (aws#22554)

Closes aws#22546. Verified via TDD with new unit test. Before the change, the unit test failed, replicating the bug in the issue with output:

```plain
FAIL test/profiling-group.test.js
  profiling group
    ✓ attach read permission to Profiling group via fromProfilingGroupArn (71 ms)
    ✓ attach publish permission to Profiling group via fromProfilingGroupName (27 ms)
    ✕ use name specified via fromProfilingGroupName (8 ms)
    ✓ default profiling group (21 ms)
    ✓ allows setting its ComputePlatform (14 ms)
    ✓ default profiling group without name (13 ms)
    ✓ default profiling group without name when name exceeding limit is generated (18 ms)
    ✓ grant publish permissions profiling group (25 ms)
    ✓ grant read permissions profiling group (25 ms)

  ● profiling group › use specified via name via fromProfilingGroupName

    expect(received).toEqual(expected) // deep equality

    Expected: "MyAwesomeProfilingGroup"
    Received: "profilingGroup"

      174 |
      175 |     const profilingGroup = ProfilingGroup.fromProfilingGroupName(stack, 'MyProfilingGroup', 'MyAwesomeProfilingGroup');
    > 176 |     expect(profilingGroup.profilingGroupName).toEqual('MyAwesomeProfilingGroup');
          |                                               ^
      177 |   });
      178 |
      179 |   test('default profiling group', () => {

      at Object.<anonymous> (test/profiling-group.test.ts:176:47)
```

----

### All Submissions:

* [Yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [No] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [N/A] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@corymhall corymhall

Labels
@aws-cdk/aws-codeguruprofiler Related to Amazon CodeGuru Profiler bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(codeguruprofiler): incorrect profiling group name is returned when using importing
3 participants
@openwebsolns @corymhall @mascur