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

Fix cloudex configuration #701

Merged
merged 1 commit into from
Feb 17, 2017
Merged

Fix cloudex configuration #701

merged 1 commit into from
Feb 17, 2017

Conversation

joshsmith
Copy link
Contributor

What's in this PR?

This fixes some Cloudex config issues that made it into a merge.

Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

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

Looking from my phone, it's hard to judge, but you might be tackling two separate things here. Look into the cloudex docs to see how they handle testing and see which parts we need to change here and which we don't.

It's possible this is all good, but I'm not sure from my phone.

if System.get_env("CLOUDEX_API_KEY") == nil do
config :cloudex, :cloudinary_api, Cloudex.CloudinaryApi.Test
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a documented Cloudex approach of enabling test mode for the API. Don't have a link right now, but should probably look into the Clodex docs to see what it does. Not sure if we wanna remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't make use of the API, so don't really need it right now.

IO.puts("NOTE: No Cloudex configuration found. Cloudex is runnning in test mode.")
config :code_corps, :cloudex, CloudexTest
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is what fixes it. The removal of the other change probably doesn't break anything right now, but might be misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm going to move this out of helpers too and into a dedicated cloudex.

@@ -1,4 +1,4 @@
defmodule Cloudex.CloudinaryApi.Test do
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably the other part of the fix.

Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

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

Considering your comments, looks good to go.

@joshsmith joshsmith merged commit de25f2e into develop Feb 17, 2017
@joshsmith joshsmith deleted the fix-cloudex-config branch February 17, 2017 20:38
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