get honey badger environment from system env var #47

jmartin-sul · 2016-09-12T16:36:25Z

instead of hard coding it. also, in a separate commit, a minor variable renaming for accuracy/consistency (happy to pull that if people think it's a bad idea).

connects to #46

ndushay · 2016-09-12T18:58:34Z

.travis.yml

@@ -11,5 +11,7 @@ notifications:
  email: false

 env:
-  secure: "lq31uKdO5F3IfUsfcmPIvmXkp2BOz56pjOAQ4S1RqHF4LoVH6LKMMd3Ocn0C9GyQfomM5NlKTRogFdJvB2iS6piXHej5iSo2/r5tmPxgX3BVMZwV6u9hEuE58U2sA+/NNwWZtivs+1U6XSRNwA2vi2qB8B5lePifCZjxsqL8l1doD4pwH+uDxXoYktberIAKizAzZFi03RvIaXHgPQYBADwrSYY4kbmrl1fL/jfdZx/ouSD055veIVJqQUGwsrdiq5lgtLSR8UfDzWZLQwDTiMLau7fJdE40lwPBAwpwDHPLdf1IGQfMih8y3YC4bQBNMi1w7J/T1nOpABvD8EZhHypYOHO/aYgWMmJM/EEzcytS52r1fieNoV3+GxxdSbOd6DN2D+hrGnZ34c3sQ8UZWXv6/PZtBBIcOsw132cQ35dOSXx78khg129IauW+RiQ5bRglZMr0Su6Pz3AL7az1CG0IDzzlJC+vmcobPLDTHJWpXIO2dyv/NyzfD0d5tYc9I1Y2/KeLUBxEm6x2GZw/dSQRih9YsZc7FXTyReNE5TQ8qGrDhpKGswr4c8MxW/3Dyktb0VwO2TlCfXwB0QKyTdd5QFSNYZ6OJyn1uPY/jHpTEzYxGAIVVFbWBwyv6NkjsIuFzhyLaOtmp6klHTdqcdFSExtIpL+8QoRmvWM06dc="
+  global:
+    - WOWZA_HONEYBADGER_ENV=travis_honeybadger_env


I believe we can do this one in travis itself, if not both of them. https://travis-ci.org/sul-dlss/dlss-wowza/settings

I use ENV variable for travis builds when I set up code coverage with CodeClimate.

sure, happy to do it in settings instead, i was just staying consistent with what was there.

should i move both and get rid of that section entirely?

now setting these env vars in project settings on travis-ci.org, removed them from .travis.yml

ndushay · 2016-09-12T18:59:11Z

src/edu/stanford/dlss/wowza/SulWowza.java

@@ -34,8 +34,8 @@
 {
    static String stacksTokenVerificationBaseUrl;
    static String stacksUrlErrorMsg = "rejecting due to invalid stacksURL property (" + stacksTokenVerificationBaseUrl + ")";
-    static final String HONEYBADGER_KEY = "WOWZA_HONEYBADGER_API_KEY";
-    static final String HONEYBADGER_ENVIRONMENT = "WOWZA_ENVIRONMENT";
+    static final String HONEYBADGER_KEY_ENV_VAR = "WOWZA_HONEYBADGER_API_KEY";


I would rename these. The names are too similar for me, and ENV_ENV makes me uncomfy.

Maybe: HB_API_KEY_NAME and HB_VM_ENV_NAME ??? with a comment saying they are environment variables???

ha, i'm always worried i'm being too verbose in my naming, but agreed, will change.

ok, the names are less similar, though i went with your earlier suggestion for the API key, and something slightly different than the above suggestion for the env name (so HONEYBADGER_API_KEY_ENV_VAR, and HONEYBADGER_ENV_NAME_ENV_VAR, respectively).

i do like the HONEYBADGER_*_ENV_VAR naming convention in general, since they're honeybadger related system environment variables, but hopefully this addresses the ENV_ENV concern (which made sense), and hopefully the immediate differences after HONEYBADGER_ make the names easier to differentiate at a glance.

ndushay · 2016-09-12T19:03:30Z

as mentioned f2f, I suggest some info should go in the README about what you have to do while developing.

jmartin-sul · 2016-09-12T19:04:37Z

pushed up the README change already

ndushay · 2016-09-12T19:06:31Z

README.md

+- `WOWZA_HONEYBADGER_API_KEY`:  You can get the API key from our [Honeybadger](https://www.honeybadger.io/) project (`DLSS-Wowza`).  Note that you may need to get added to the `DLSS-Wowza` project as a user in order to obtain the key (someone from Devops should be able to add you).
+- `WOWZA_HONEYBADGER_ENV`:  Anything other than the names of the deployed environments should be fine.  E.g., something like `dlss_wowza_laptop_$USER`.
+
+If the environment variables are not set, your tests will fail.



…r name (HONEYBADGER_KEY -> HONEYBADGER_API_KEY_ENV_VAR). update tests.

…. remove from .travis.yml.

…hard coding * rename HONEYBADGER_ENVIRONMENT to HONEYBADGER_ENV_NAME_ENV_VAR (since we get the environment string from the env var now) * test new behavior (analogous to honeybadger API key test) * update README

coveralls · 2016-09-12T19:52:10Z

Coverage increased (+0.05%) to 97.321% when pulling 3733abd on honeybadger-report-prod-only into 4186f35 on master.

jmartin-sul · 2016-09-12T19:59:57Z

ok, took a shot at addressing the concerns raised above

ndushay · 2016-09-12T20:51:50Z

src/edu/stanford/dlss/wowza/SulWowza.java

@@ -156,17 +156,23 @@ public void initHoneybadger()
        if(environment == null)
            environment = new SulEnvironment();

-        String apiKey = environment.getEnvironmentVariable(HONEYBADGER_KEY);
+        String apiKey = environment.getEnvironmentVariable(HONEYBADGER_API_KEY_ENV_VAR);
+        String honeybadgerEnv = environment.getEnvironmentVariable(HONEYBADGER_ENV_NAME_ENV_VAR);


technically, these could be considered static constants. But I don't really mind them as is.

well... normally i'd be in favor of making stuff constant where possible... but in this case, i think we can't do that at class declaration time, because the values come from environment, which doesn't get initialized until object instantiation.

ndushay · 2016-09-12T20:52:28Z

One last minor line comment. It's okay if you disagree.

jmartin-sul added the in progress label Sep 12, 2016

jmartin-sul mentioned this pull request Sep 12, 2016

dlss-wowza plugin: only notify w honeybadger for prod #46

Open

jmartin-sul force-pushed the honeybadger-report-prod-only branch 3 times, most recently from 4f13162 to c9d023b Compare September 12, 2016 18:04

ndushay reviewed Sep 12, 2016
View reviewed changes

jmartin-sul force-pushed the honeybadger-report-prod-only branch from c9d023b to 9aa27f4 Compare September 12, 2016 18:58

ndushay reviewed Sep 12, 2016
View reviewed changes

more accurate name for constant that holds honeybadger api key env va…

520cde7

…r name (HONEYBADGER_KEY -> HONEYBADGER_API_KEY_ENV_VAR). update tests.

jmartin-sul force-pushed the honeybadger-report-prod-only branch 2 times, most recently from 93a1ed0 to 04e7429 Compare September 12, 2016 19:44

jmartin-sul added 2 commits September 12, 2016 12:48

set env vars for travis in travis project settings (on travis-ci.org)…

089b7fc

…. remove from .travis.yml.

jmartin-sul force-pushed the honeybadger-report-prod-only branch from 04e7429 to 3733abd Compare September 12, 2016 19:48

ndushay reviewed Sep 12, 2016
View reviewed changes

ndushay merged commit 7c67fc4 into master Sep 12, 2016

ndushay deleted the honeybadger-report-prod-only branch September 12, 2016 21:13

ndushay removed the in progress label Sep 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get honey badger environment from system env var #47

get honey badger environment from system env var #47

jmartin-sul commented Sep 12, 2016

ndushay Sep 12, 2016

jmartin-sul Sep 12, 2016

jmartin-sul Sep 12, 2016

ndushay Sep 12, 2016 •

edited

jmartin-sul Sep 12, 2016

jmartin-sul Sep 12, 2016

ndushay commented Sep 12, 2016

jmartin-sul commented Sep 12, 2016

ndushay Sep 12, 2016

coveralls commented Sep 12, 2016

jmartin-sul commented Sep 12, 2016

ndushay Sep 12, 2016

jmartin-sul Sep 12, 2016

ndushay commented Sep 12, 2016

get honey badger environment from system env var #47

get honey badger environment from system env var #47

Conversation

jmartin-sul commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay Sep 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay commented Sep 12, 2016

jmartin-sul commented Sep 12, 2016

Choose a reason for hiding this comment

coveralls commented Sep 12, 2016

jmartin-sul commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay commented Sep 12, 2016

ndushay Sep 12, 2016 •

edited