add templates for a few magic links#69
Conversation
fxkr
left a comment
fxkr
left a comment
There was a problem hiding this comment.
Thank you very much! I appreciate the contribution.
I have a few code review comments though.
| "codebuild": { // AWS CodeBuild | ||
| "build": null, | ||
| "project": null, | ||
| "project": () => `https://${this.region}.${this.console}/codesuite/codebuild/projects/${this.resource}`, |
There was a problem hiding this comment.
Can you add unit tests for all the resource types that you are adding support for?
It's easy to do, existing tests are here: https://github.com/link2aws/link2aws.github.io/blob/master/testcases/aws.json
| "": null, | ||
| "": () => { | ||
| const parts = this.resource.split('/'); | ||
| const myid = parts[2]; |
There was a problem hiding this comment.
Why myid? This could be just id, or could be more specific (maybe api_id). But my is just visual clutter that doesn't convey information and should be removed.
|
|
||
| // Running as command line script? (not in browser, and not as library) | ||
| /* istanbul ignore if */ | ||
| if (typeof (require) !== 'undefined' && require.main === module) { |
There was a problem hiding this comment.
Why are you removing all this code?
| }, | ||
| "acm": { // AWS Certificate Manager | ||
| "certificate": () => `https://${this.console}/acm/home?region=${this.region}#/?id=${this.resource}`, | ||
| "certificate": () => `https://${this.console}/acm/home?region=${this.region}#/certificates/${this.resource}`, |
There was a problem hiding this comment.
Thanks. I have verified that this change is correct.
However, you also need to update the unit tests. Currently they are failing due to this change. Search for arn:aws:acm:us-east-1:123456789012:certificate/1f6ee793-4064-4a10-9567-f03875640b35 in https://github.com/link2aws/link2aws.github.io/blob/master/testcases/aws.json - the console URL there is currently outdated.
| "": null, | ||
| "": () => { | ||
| const parts = this.resource.split('/'); | ||
| const myid = parts[2]; |
There was a problem hiding this comment.
Can you fix all the whitespace errors?
- There is trailing whitespace at the end of the
const myid = ...line. Please remove it. - The
returnline is indented with a mix of tabs and spaces. It should be spaces only. - The closing
}has the wrong indent level. It should be indented one level less.
| "apigateway": { // Manage Amazon API Gateway | ||
| "": null, | ||
| "": () => { | ||
| const parts = this.resource.split('/'); |
There was a problem hiding this comment.
I understand why you did this, but this is the wrong solution. Actually what has happened is that you uncovered a bug (very nice! Thank you!). The ARN was parsed via the // ...:resource-id code branch in the constructor() function, but should have been parsed via the // ...:resource-type/resource-id (resource-id can contain slashes!) code branch. Instead of splitting here, can you just change > 0 to >= 0 in the the existing code - from this:
// ...:resource-type/resource-id (resource-id can contain slashes!)
else if (typeof (tokens[5]) != 'undefined' && tokens[5].indexOf('/') > 0) {
to this:
// ...:resource-type/resource-id (resource-id can contain slashes!)
else if (typeof (tokens[5]) != 'undefined' && tokens[5].indexOf('/') >= 0) {
Then you can simply use the "": () => `...`, pattern here.
I have verified that all existing tests continue to pass with that change. So it really is a case that wasn't exercised before.
2 participants
adding a few templates for links
acm cert
apigateway
codebuild project
eip-allocation
ecr repository
ecs environment
es domain
glue job
rds snapshot