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

Code testing for existence environment variables halts validation, even when those variables are passed #96

Closed
jkeefe opened this issue Jan 28, 2017 · 4 comments
Assignees

Comments

@jkeefe
Copy link

jkeefe commented Jan 28, 2017

Please use GitHub issues only to report bugs. To ask a general question or request assistance/support, please use the Claudia.js Gitter Chat instead.

To report a bug or a problem, please fill in the sections below. The more you provide, the better we'll be able to help.


  • Expected behaviour:

When deploying with environment variables (using --set-env or --set-env-from-json ) , I expect those variables to be considered in the validation process.

  • What actually happens:

During package validation, any code that tests for the existence of environment variables throws an error -- so the validation and the deployment fails.

  • Link to a minimal, executable project that demonstrates the problem:

https://github.com/jkeefe/claudia-env-trouble-demo

  • Steps to install the project:
  • Download or clone
  • run npm install
  • Steps to reproduce the problem:

Run:

claudia create --handler lambda.handler --set-env WIT_SERVER_ACCESS_TOKEN=ABC123 --deploy-proxy-api --region us-east-1 --name claudia-test

My error looks like this:

validating package
Error: No wit.ai API token specified
    at module.exports (/private/var/folders/bc/pnjfps4j3rsgg6lc638p8nw80000gn/T/24cee1de-8e32-4348-a219-a6b6840fc571/node_modules/botkit-middleware-witai/src/botkit-middleware-witai.js:6:15)
    at Object.<anonymous> (/private/var/folders/bc/pnjfps4j3rsgg6lc638p8nw80000gn/T/24cee1de-8e32-4348-a219-a6b6840fc571/answerbot/index.js:1:107)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/private/var/folders/bc/pnjfps4j3rsgg6lc638p8nw80000gn/T/24cee1de-8e32-4348-a219-a6b6840fc571/server.js:4:17)
cannot require ./lambda after clean installation. Check your dependencies.

The wit-ai package (and several packages I use) tests to make sure variables are passed. If they don't exist, or are null, they throw an error. These errors are getting thrown in the Claudia validation process even when I set them.

To get it to deploy, I had to add "fake" variables throughout my code to pass the not-null tests for the environment variable. So adding || "temp" on the erroring line clears the validation:

var wit = require('botkit-middleware-witai')({
    token: process.env.WIT_SERVER_ACCESS_TOKEN || "temp"
});
@gojko gojko self-assigned this Jan 28, 2017
@gojko
Copy link
Member

gojko commented Jan 28, 2017

Your suggestion sounds as a nice idea how to solve this. Would you like to try to give this a shot? I could guide you where to implement the changes.

@jkeefe
Copy link
Author

jkeefe commented Jan 29, 2017

I can certainly try! Would love the challenge.

@gojko
Copy link
Member

gojko commented Jan 29, 2017

  • the code change is most likely to go here, just before the require call for the API Module:
    apiModule = require(path.join(dir, apiModulePath));
  • you should use this utility to read out the env vars from the options: https://github.com/claudiajs/claudia/blob/master/src/util/read-env-vars-from-options.js - this will return an object containing {Variables: <your vars as a hash map>}.
  • After that, it should be just a matter of setting process.env from the options.
  • You should add a test to https://github.com/claudiajs/claudia/blob/master/spec/validate-package-spec.js proving that the new code works as expected. I suggest creating a simple project in spec/test-projects that throws an exception if some env var isn't defined, then proving that it loads with the vars set in options, and that it throws an exception without the vars set in options. Just follow the examples for the other test projects there.
  • I suggest you create another util function in the /src/util directory that will load up the environment vars (using the other utility function) and populate process.ENV. Then you can create a nice separate test for that function and run it in isolation.
  • The full test package takes about 2 hours, so don't bother running everything, but please run at least the specs for validatePackage, and your new utility function before creating a pull request. The contributors guide explains how to run just selected tests

@gojko gojko closed this as completed in 3e16fa6 Feb 18, 2017
@gojko
Copy link
Member

gojko commented Feb 18, 2017

this is now on NPM as 2.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants