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

Show resource names instead of IDs #90

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

s12v
Copy link
Contributor

@s12v s12v commented Nov 6, 2020

Issue #: #29 (partially)

Description of changes:
Use resource name if available, fall back to ID.

Note: there's no tests, I'm not entirely sure if it works as expected, please check.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ConnorKirk
Copy link
Contributor

Hi @s12v
Thanks for this PR!
This is a feature we would like to add.

We intend to test this feature. We will come back to you with what we find.

@svozza
Copy link
Contributor

svozza commented Nov 19, 2020

So I've looked into this a bit more and unfortunately this won't probably have the effect required, the reason why is that resourceName and resourceId come from AWS Config and for the items we're interested in like VPCs, subnets etc, Config only gives us the resourceId. However, for the other items like Lambda, that actually have a resourceName, it's normally the same as resourceId so it doesn't matter which we use. I'm still inclined to merge your pull request because I don't think it will do any harm but I don't think it will make an appreciable difference in the way we display items on the front end. This will require more thought on our side.

@s12v
Copy link
Contributor Author

s12v commented Nov 23, 2020

@svozza, it's intended to cover some resources, that have resourceName and it's more descriptive than ID. I had in mind RDS instances and SQS queues.

Right now, looking at DB instances, it's a long list of items like

db-ODVUQ7RJMOX4C3............
db-XXXXXXXXXXXXXX............
db-XXXXXXXXXXXXXX............

For example looking at the Neptune instance used by Perspective, it has these properties in the Config JSON:

"resourceId": "db-ODVUQ7RJMOX4C3............",
"resourceName": "neptunedbinstance-gebrq......",

While neptunedbinstance-gebrq...... is not the most descriptive name, I think it's much better then the ID.

@svozza
Copy link
Contributor

svozza commented Nov 23, 2020

Yep, it's definitely an improvement. I would prefer an implementation that relies on a null check rather than using the in keyword (no need to worry about prototype chains etc). Would you be willing to update the PR to look something like this?

const buildGeneric = ({properties}) => {
    properties.title = R.defaultTo(properties.resourceId, properties.resourceName);
}

Alternatively I can take over the PR (there's a few other changes in the file I'd like to make).

@s12v
Copy link
Contributor Author

s12v commented Nov 23, 2020

@svozza, updated the PR. Thanks. You can of course take over the PR if you'd like.

@svozza
Copy link
Contributor

svozza commented Nov 23, 2020

Let's merge this actually and I can make changes in a separate PR. Thank you!

@svozza svozza merged commit bfa9980 into aws-solutions:master Nov 23, 2020
@svozza svozza added the v1.1.0 label Nov 23, 2020
@s12v s12v deleted the 29-show-resource-names branch November 24, 2020 10:49
@toksvaeth toksvaeth added the enhancement New feature or request label Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v1.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants