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

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

Open
kommen opened this issue Jul 21, 2016 · 24 comments
Open

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

kommen opened this issue Jul 21, 2016 · 24 comments

Comments

@kommen
Copy link

kommen commented Jul 21, 2016

I'm using this goth branch, which looks like it might be merged soonish: peburrows/goth#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
Copy link
Author

kommen commented Jul 21, 2016

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

@sashaafm
Copy link
Collaborator

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
Copy link

tazjin commented Aug 2, 2016

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
Copy link

tazjin commented Aug 2, 2016

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 added a commit to tazjin/gcloudex that referenced this issue Aug 2, 2016
Instead of parsing the Goth application configuration, retrieve
the project ID from the Goth.Config process.

This does not currently work with how get_project_id is called at
compile time in the rest of the project.

Please see issue Overbryd#4 for more information.
@sashaafm
Copy link
Collaborator

sashaafm commented Aug 2, 2016

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

@sashaafm
Copy link
Collaborator

sashaafm commented Aug 2, 2016

@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
Copy link

tazjin commented Aug 2, 2016

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

@sashaafm
Copy link
Collaborator

sashaafm commented Aug 3, 2016

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

@tazjin
Copy link

tazjin commented Aug 4, 2016

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

@sashaafm
Copy link
Collaborator

sashaafm commented Aug 4, 2016

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

@tazjin
Copy link

tazjin commented Aug 4, 2016

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
Copy link
Collaborator

sashaafm commented Aug 4, 2016

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
Copy link

tazjin commented Aug 4, 2016

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

@sashaafm
Copy link
Collaborator

sashaafm commented Aug 6, 2016

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
Copy link

tazjin commented Aug 6, 2016

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

hamann added a commit to nextjournal/gcloudex that referenced this issue Sep 4, 2017
hamann added a commit to nextjournal/gcloudex that referenced this issue Sep 4, 2017
hamann added a commit to nextjournal/gcloudex that referenced this issue Sep 4, 2017
@jdrakes
Copy link

jdrakes commented Jan 10, 2018

Is this issue still being worked on?

@sashaafm
Copy link
Collaborator

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
Copy link
Owner

Overbryd commented Jul 12, 2018

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
Copy link
Collaborator

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

@Overbryd
Copy link
Owner

Overbryd commented Jul 12, 2018 via email

@sashaafm
Copy link
Collaborator

@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
Copy link
Collaborator

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

@Overbryd
Copy link
Owner

Overbryd commented Jul 13, 2018 via email

@Overbryd
Copy link
Owner

Overbryd commented Aug 9, 2018

@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

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

No branches or pull requests

5 participants