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

Avoid name clashing #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mstn
Copy link
Contributor

@mstn mstn commented Dec 4, 2019

Hi, I added a random string to component name in order to avoid name clashing. I followed the same approach as for the "official" dynamodb component.

@mstn
Copy link
Contributor Author

mstn commented Mar 10, 2020

@bboure did you have any chance to review this pr?

Copy link
Owner

@bboure bboure left a comment

Choose a reason for hiding this comment

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

Thank you @mstn and sorry for late review. I must have missed it.

As you can tell, I haven't been working on this for a long time.

In what case would there be a clash in the naming?
Is this in the case a domain already exists with the same name?

if so, wouldn't this line re-use it?
If I remember well, the idea was to be able to manage an existing domain.

Do you see a reason why this would be a bad idea?

and Thank you for your contrib!

const prevEbsOptions = pick(prevDomain.ebsOptions, ['EBSEnabled', 'VolumeSize', 'VolumeType']);

const clusterConfig = pick(domain.elasticsearchClusterConfig, ['InstanceCount', 'InstanceType']);
const prevClusterConfig = pick(prevDomain.elasticsearchClusterConfig, ['InstanceCount', 'InstanceType']);
Copy link
Owner

Choose a reason for hiding this comment

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

@mstn What is behind this?
are there any other "options" that we would need to compare under ebsOptions and elasticsearchClusterConfig ?

@mstn
Copy link
Contributor Author

mstn commented Mar 10, 2020

The problem is when a new es domain is created. It is new for the "local" state, but it might exist on the AWS account (different project, different env...).

If it is already present in the local state, then the existing name is used.

As far as I can see, the usual approach is to add a random string to the name.

@mstn
Copy link
Contributor Author

mstn commented Mar 10, 2020

@bboure perhaps we should hold on. I think they changed how resource names are created. Previously, in many cases, a random string was added to the given name (dynamodb, lambdas, buckets...). Now, the given name is not changed. Let me understand what is the official way.

@bboure
Copy link
Owner

bboure commented Mar 11, 2020

ok thanks @mstn
Let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants