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

region-info: Fact.regions behavior does not match description. #27260

Closed
chamsco-aws opened this issue Sep 22, 2023 · 2 comments · Fixed by #27506
Closed

region-info: Fact.regions behavior does not match description. #27260

chamsco-aws opened this issue Sep 22, 2023 · 2 comments · Fixed by #27506
Labels
@aws-cdk/region-info Related to AWS Region information bug This issue is a bug. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2

Comments

@chamsco-aws
Copy link

chamsco-aws commented Sep 22, 2023

Describe the bug

In fact.ts

/**
* @returns the list of names of AWS regions for which there is at least one registered fact. This
* may not be an exhaustive list of all available AWS regions.
*/
public static get regions(): string[] {
// Return by copy to ensure no modifications can be made to the undelying constant.
return Array.from(AWS_REGIONS);
}

the description of regions it states it returns any region that has at least one fact registered. This is untrue, it is only listing from the fixed list defined in AWS_REGIONS from aws-entities.ts.

Either the description needs to be updated, or the logic needs to pull the list of regions from this.database instead of the AWS_REGIONS export.

Expected Behavior

  1. Call Fact.regions and save the array as call1
  2. Register a new fact with a region that has yet to be added to the list in AWS_REGIONS
  3. Call Fact.regions as call2
  4. The call2 array would include the name of my newly added region and the call1 array would not.

Current Behavior

  1. Call Fact.regions and save the array as call1
  2. Register a new fact with a region that has yet to be added to the list in AWS_REGIONS
  3. Call Fact.regions as call2
  4. The call2 array is identical to the call1 array.

Reproduction Steps

  it('can find new region in `Fact.region`', () => {
    Fact.register({
      region: 'us-unreleased-1',
      name: FactName.PARTITION,
      value: 'aws',
    });
    expect(Fact.regions.includes('us-unreleased-1')).toBeTruthy();
  })

Possible Solution

Switch the code from Array.from(AWS_REGIONS) to Array.from(Object.keys(this.database)) or update the documentation to say it is a fixed list based on AWS_REGIONS in aws-entities.ts

Additional Information/Context

No response

CDK CLI Version

2.95.0 (build cfa7e88)

Framework Version

No response

Node.js Version

v18.17.1

OS

OSX 13.5.2 (22G91)

Language

Typescript

Language Version

TypeScript (4.3.0)

Other information

You can reach out to me on the Amazon internal slack as @chamsco

@chamsco-aws chamsco-aws added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 22, 2023
@github-actions github-actions bot added the @aws-cdk/region-info Related to AWS Region information label Sep 22, 2023
@peterwoodworth peterwoodworth added 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 Sep 25, 2023
@peterwoodworth
Copy link
Contributor

Thanks for the report, makes sense!

I'm not personally familiar with why you would want to use this feature (returning regions from Fact), so I'm not sure if we should just update the documentation or adjust the functionality to match the documentation. I would think it would make sense to adjust the functionality, but there could be something I'm missing. Either way, it should be simple to address

@peterwoodworth peterwoodworth added needs-review documentation This is a problem with documentation. and removed needs-review labels Sep 25, 2023
@mergify mergify bot closed this as completed in #27506 Oct 24, 2023
mergify bot pushed a commit that referenced this issue Oct 24, 2023
Facts are only being returned from the regions in a constant list.
This PR returns facts for all Regions in `AWS_REGIONS` + `this.database`.

Closes #27260.

----

*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.

mrgrain pushed a commit that referenced this issue Nov 1, 2023
Facts are only being returned from the regions in a constant list.
This PR returns facts for all Regions in `AWS_REGIONS` + `this.database`.

Closes #27260.

----

*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
Labels
@aws-cdk/region-info Related to AWS Region information bug This issue is a bug. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants