cloudfoundry-community / node-cfenv

easy access to your Cloud Foundry application environment for node
Apache License 2.0
74 stars 20 forks source link

Allow local `vcap` options to be supplied in a file, not just an object #31

Closed jsmoorman closed 6 years ago

jsmoorman commented 6 years ago

Would it be possible to load everything defined under env: in the manifest.yml to be loaded into process.env in the constructor when isLocal is true?

This would allow me to access my environment variables defined in the manifest.yml without having to use another library or any additional code.

pmuellr commented 6 years ago

That sounds interesting, but I think I'll have to noodle on it.

I'm hesitant to reset env vars magically, since manifest.yml is currently only used in "local" mode, and only to get the app name. To use values from the manifest more widely, we'd have to make it an opt-in thing, otherwise it would break existing apps.

I think if that functionality were to be added, it would have to be opt-in, like a new boolean option on getAppEnv() called something like setManifestEnvIfLocal or such.

jsmoorman commented 6 years ago

Thank you for the response.

Upon further work with Cloud Foundry and this module, I started using user-defined services to store my credentials instead of the env variables. But this now presents another problem.

I understand how to read my credentials locally by passing

{
  vcap: {
    services: {
      "user-provided": [
        {
          "credentials": {
            "password": "<password>",
            "username": "<username>"
          },
          "name": "<custom-service>",
        }
      ]
    }
  }
}

to cfenv.getAppEnv(), but I run into the issue of having to either:

I like the second bullet but I dislike having to either wrap the require('<filename>.json') in a try/catch like you suggest in a comment you left in #6 or pushing this untracked file to Cloud Foundry so the app doesn't complain it can't find it only to ignore it anyways.

Is there a way to allow the options parameter passed to getAppEnv() to also be a string, and when it is, it's assumed to be a path to a .json that can be parsed when running locally?

pmuellr commented 6 years ago

So maybe something like a vcapFile property on the getAppEnv() options object, which would be a filename of a JSON file with application and services properties, and only read and used when running locally?

That sounds fairly reasonable.

pmuellr commented 6 years ago

@jsmoorman I have an implementation of this in PR https://github.com/cloudfoundry-community/node-cfenv/pull/33 ; see notes there about testing with this implementation. I'd like to find out if this works for you.

jsmoorman commented 6 years ago

So I've just tested it and it works amazingly!

I've exported the VCAP_SERVICES from my Cloud Foundry App on Bluemix and just wrapped it with:

{
  "services": "<exported VCAP_SERVICES>"
}

The only possible suggestion I have is to allow users to use the exported json itself as the VCAP_SERVICES instead of as the whole app environment so that users don't have to wrap the export as I did above -- but then you would lose the ability to customize the VCAP_APPLICATION.

That is, unless you allowed them to use it in tandem with the vcap field in the options argument, but then you have the possibility of users providing VCAP_SERVICES through the vcapFile and through the vcap fields.

jsmoorman commented 6 years ago

But to answer your original question, this works exactly as expected and provides the functionality I needed.

Thank you!

pmuellr commented 6 years ago

The only possible suggestion I have is to allow users to use the exported json itself as the VCAP_SERVICES instead of as the whole app environment so that users don't have to wrap the export as I did above

Ya, I can see that, but I think only if such a 'services-only' JSON file would be useful in some other environment. Otherwise, it's kinda a one-off thing anyway, so I don't see the extra services property indirection as too bad.

It would also not be parallel to the existing vcap option.

Probably worth noodling on whether there should be vcapServices and vcapServicesFile options that would just have the "services" bits and not "application" bits, especially if it turns out the application bits rarely need to be set like this given the default processing.

But let me go ahead and publish what I have now, I think this weekend or next week. Tied up for now and I want to have some time ready if for some unknown reason this causes existing users problems, so I can fix that.

Thanks a bunch for trying this out!