gcloudex icon indicating copy to clipboard operation
gcloudex copied to clipboard

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

Open kommen opened this issue 9 years ago • 24 comments

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 avatar Jul 21 '16 17:07 kommen

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

kommen avatar Jul 21 '16 17:07 kommen

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.

sashaafm avatar Jul 21 '16 17:07 sashaafm

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 avatar Aug 02 '16 15:08 tazjin

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.

tazjin avatar Aug 02 '16 16:08 tazjin

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

sashaafm avatar Aug 02 '16 17:08 sashaafm

@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?

sashaafm avatar Aug 02 '16 20:08 sashaafm

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

tazjin avatar Aug 02 '16 21:08 tazjin

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

sashaafm avatar Aug 03 '16 21:08 sashaafm

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

tazjin avatar Aug 04 '16 06:08 tazjin

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

sashaafm avatar Aug 04 '16 09:08 sashaafm

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.

tazjin avatar Aug 04 '16 11:08 tazjin

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.

sashaafm avatar Aug 04 '16 13:08 sashaafm

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

tazjin avatar Aug 04 '16 14:08 tazjin

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.

sashaafm avatar Aug 06 '16 13:08 sashaafm

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

tazjin avatar Aug 06 '16 14:08 tazjin

Is this issue still being worked on?

jdrakes avatar Jan 10 '18 22:01 jdrakes

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.

sashaafm avatar Jan 11 '18 13:01 sashaafm

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?

Overbryd avatar Jul 12 '18 18:07 Overbryd

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

sashaafm avatar Jul 12 '18 19:07 sashaafm

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 [email protected] 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.

Overbryd avatar Jul 12 '18 19:07 Overbryd

@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 avatar Jul 13 '18 10:07 sashaafm

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

sashaafm avatar Jul 13 '18 10:07 sashaafm

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

I will take good care.

Sasha Fonseca [email protected] 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 avatar Jul 13 '18 12:07 Overbryd

@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:

  • I would also prepare for a new hex release, so that I can get my projects off the git: dependency
  • I would notify all other forks to join forces back to the mainline

Overbryd avatar Aug 09 '18 09:08 Overbryd