mongodb / awscdk-resources-mongodbatlas

MongoDB Atlas AWS CDK Resources
Apache License 2.0
33 stars 15 forks source link

chore: Rename clusters folder to cluster for L1 cluster resource and syncs with CFN #277

Closed lantoli closed 1 month ago

lantoli commented 1 month ago

Rename clusters folder to cluster for L1 cluster resource and syncs with CFN

Jira ticket: CLOUDP-248306

Link to any related issue(s):

Type of change:

Required Checklist:

Further comments

lantoli commented 1 month ago

waiting for team decision before merging

lockenj commented 1 month ago

This fix breaks backwards compatibility. Yesterday on version v3.5.1 the below snippet worked.

this.#_atlasCluster.connectionStrings.standardSrv

Now in v3.5.2 I get error:

TypeError: Cannot read properties of undefined (reading 'standardSrv')
lockenj commented 1 month ago

It appears that the visibility of this property was changed.

From:

public readonly connectionStrings: ConnectionStrings;

To:

readonly connectionStrings?: ConnectionStrings;
lockenj commented 1 month ago

I have created issue 278 to track.

For now I will have to fix my version to 3.5.1

lantoli commented 1 month ago

sorry @lockenj for the inconvenience.

that property shouldn't have existed in the first place. It was manually added so it was not synced with underlaying CFN resource. This CDK L1 resource must be generated automatically.

If you need to access those properties, you can do things like:

this.#_atlasCluster.getAtt("ConnectionStrings.StandardSrv")

you can take this PR code change as a reference.

thanks

romulus-ai commented 1 month ago

Thanks für the fast response, @lantoli your fix works very well! However, would be nice to access the connection strings of a generated cluster in an easier way (i.e. like before). Additionally may if you guys break backwards compatibility try not to make a patch release :)

lantoli commented 1 month ago

I agree about accessing in an easier way but this is more a CDK limitation in L1 constructs.

I understand your point about breaking compatibility and will review it for future changes, in this case we thought of this as a fix as that attribute shouldn't have existed in the first place.

Thanks for your understanding, any other question please let us know