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

credentials for services migrated to iam #18

Conversation

dpopp07
Copy link

@dpopp07 dpopp07 commented May 21, 2018

@germanattanasio I spoke to Taj the other day about this issue. I just took a quick pass at it while I had a free minute. We may not be able to rely on the resource_name in credentials so this may not be ideal, but it seemed to be that was the best way to support the migration. Let me know what you think.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 92.157% when pulling 873cf9f on dpopp07:dp/credentials-for-migrated-services into 412d549 on germanattanasio:master.

var instance = services[service_name][i];
var usingResourceName = instance.credentials && instance.credentials.resource_name;
if ((usingResourceName && instance.credentials.resource_name === name) &&
(!iname || iname === instance.name)
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't you check

if (instance.credentials.resource_name === name && (!iname || iname === instance.name)) {
  // ...
}

Copy link
Author

Choose a reason for hiding this comment

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

I was just trying to be safe. In the case that credentials isn't a key on the object, that would cause the module to crash. But I can make that change

@germanattanasio
Copy link
Owner

@dpopp07 This looks good.
Here are some tasks to address before merging this PR:

  • Migrate a Discovery or Conversation CF instance and check if the JSON matches the one you are expecting.
  • Add a section in the README about migrated CF services.
  • Add more tests for corner cases where some fields may not exist. Ideally, the new code coverage should be higher than the previous one.

@dpopp07
Copy link
Author

dpopp07 commented Jun 7, 2018

Closing this PR, as ibmcloud-link seems to be going away. I can re-open if the need arises

@dpopp07 dpopp07 closed this Jun 7, 2018
@germanattanasio
Copy link
Owner

Thanks for the work @dpopp07

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

3 participants