Overbryd / gcloudex

Friendly set of wrappers for Google Cloud Platform services' API's in Elixir.
64 stars 39 forks source link

Don't require :goth :json config to be present at compile time #4

Open kommen opened 8 years ago

kommen commented 8 years ago

I'm using this goth branch, which looks like it might be merged soonish: https://github.com/peburrows/goth/pull/6

This doesn't require the :goth :json to be set at all, as it can be discovered.

However, right now if :json is not set, gcloudex doesn't compile:

==> gcloudex
Compiling 11 files (.ex)

== Compilation error on file lib/cloud_storage/client.ex ==
** (ArgumentError) argument error
    :erlang.iolist_to_binary(nil)
    lib/poison/parser.ex:35: Poison.Parser.parse/2
    lib/poison/parser.ex:50: Poison.Parser.parse!/2
    lib/poison.ex:83: Poison.decode!/2
    lib/gcloudex.ex:14: GCloudex.get_project_id/0
    lib/cloud_storage/client.ex:2: (module)

could not compile dependency :gcloudex, "mix compile" failed. You can recompile this dependency with "mix deps.compile gcloudex", update it with "mix deps.update gcloudex" or clean it with "mix deps.clean gcloudex"```
kommen commented 8 years ago

Just saw #2, which is about the same error, but I believe with goth supporting Google Cloud Platform metadata this should be reconsidered?

sashaafm commented 8 years ago

Hello @kommen, yes I saw that new goth branch yesterday.

I'll have to look into it. I'll try to take a look this weekend but I've got a ton of work until the following week so I can't promise anything.

tazjin commented 7 years ago

Hey,

let me know if you need any help / clarification. I'm fixing up the GCP metadata branch today to get it merged soon.

tazjin commented 7 years ago

I took a quick look at what's causing this. Fixing it in the function GCloudex.get_project_id is no problem, however it seems like the project is set up in a way that expects the project ID to be present at compile time due to the heavy macro usage.

I would suggest deferring those to runtime as this can cause other problems as well, e.g. identical artifacts being deployed to test and production environments.

To retrieve the project ID from Goth you should call Goth.Config.get(:project_id) instead of reading it from the configuration, once the Cloud Metadata pull request is merged that field will be populated from the metadata service as well.

sashaafm commented 7 years ago

Sorry I didn't look at this in the meantime. I'll take a look later today!

sashaafm commented 7 years ago

@tazjin could you please elaborate on deferring the project ID to be present at runtime? Do you mean in the module attribute in the Request and Impl modules?

Should this attribute be changed so that the function get_project_id is always called directly at runtime?

tazjin commented 7 years ago

Yes, exactly. The project ID may be different at runtime than at compile time

sashaafm commented 7 years ago

@tazjin I'll try to take care of this in the weekend or are you taking care of it in your fork?

tazjin commented 7 years ago

@sashaafm My fork now supports the same Goth.Config.get(:project_id) call when running via GCP metadata :)

sashaafm commented 7 years ago

Nice, will you make a PR? That's the only change right?

tazjin commented 7 years ago

Nope, there's work left to do - I'm wondering if you could explain what the idea with the macro utilisation is? If it was only for embedding the project ID I would remove that.

sashaafm commented 7 years ago

Yes just for embedding the project ID for easier access. I guess I should have thought a bit more about that design decision! Removing it from the module attribute to the function for runtime access shouldn't break anything I think.

tazjin commented 7 years ago

I guess stripping the whole .Impl thing and just putting the code directly in the modules is feasible then?

sashaafm commented 7 years ago

I think so. I was inspired by ExAws so I decided to follow their structure when I started building GCloudex. However, GCloudex don't really uses macros to the same extent as ExAws does.

tazjin commented 7 years ago

I stripped them out in my fork but the tests need some restructuring as well, haven't had time for that so far.

jdrakes commented 6 years ago

Is this issue still being worked on?

sashaafm commented 6 years ago

Hello @jdrakes, I'm maintaining or working on this project actively anymore and probably won't work on it again, unless I've got the need to do so. However, I'm open to contributions.

Overbryd commented 6 years ago

So @sashaafm / @hamann made a couple of solid contributions, fixing this library.

Would you please merge his changes or retire this project or hand it over to someone who is actively maintaining it?

sashaafm commented 6 years ago

@Overbryd indeed I’m not maintaining this anymore. Are you interested?

Overbryd commented 6 years ago

Thanks for the offer. I am happy to take it.

Lets figure out the handover in a PM.

On 12. Jul 2018, at 21:23, Sasha Fonseca notifications@github.com wrote:

@Overbryd https://github.com/Overbryd indeed I’m not maintaining this anymore. Are you interested?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sashaafm/gcloudex/issues/4#issuecomment-404622912, or mute the thread https://github.com/notifications/unsubscribe-auth/AABSd34FifTJaKgghWdWkyVorwSj143Sks5uF6IegaJpZM4JSDDW.

sashaafm commented 6 years ago

@Overbryd I'm going to transfer the ownership to you. Please take good care of this lib, since it's got some sentimental value for me (was related to my Master Thesis). Unfortunately time goes on and I I've got little of it to dedicate to this library.

sashaafm commented 6 years ago

@Overbryd please delete your fork, otherwise GitHub doesn't allow the ownership transfer.

Overbryd commented 6 years ago

Will do. I have some time this weekend to complete the take over.

I will take good care.

Sasha Fonseca notifications@github.com schrieb am Fr. 13. Juli 2018 um 12:24:

@Overbryd https://github.com/Overbryd please delete your fork, otherwise GitHub doesn't allow the ownership transfer.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sashaafm/gcloudex/issues/4#issuecomment-404793029, or mute the thread https://github.com/notifications/unsubscribe-auth/AABSd2xV1OiZgReuM0g7B35tylChXxHUks5uGHVmgaJpZM4JSDDW .

Overbryd commented 5 years ago

@sashaafm I have removed the fork, and I have now accumulated all the changes to the configuration to have this project run again on my local version.

If you still want to transfer the project, you can do so now, as my fork is now removed.

Next steps: