search

danger / peril

☢️ Serious and immediate danger.
https://danger.systems
MIT License
461 stars 58 forks source link

environment variable marked in `peril.settings.json` are not passed to runner #474

Closed pieh closed 4 years ago

pieh commented 4 years ago

Hi, we recently updated our rather outdated peril instance running on heroku to latest and the env vars no longer get passed to runners (when trying to access via peril.env.ENV_VAR_NAME). In fact when we do import { peril } from "danger", peril is just empty object (which I guess is default set in https://github.com/danger/danger-js/blob/88f242b7a21001cd2c83b87554c738769d2aa706/source/runner/Dangerfile.ts#L137-L146 )

I have code change that seems to fix that - https://github.com/danger/peril/compare/master...pieh:fix/pass-env-vars-to-runner (sorry for formatting mess - it seems to be forced vscode format on save)

The actual change is just dangerRuntimeEnv.sandbox related parts - there is no sandbox field there - and maybe that is the actual bug and not the fact that code tries to access it.

Because I lack context on inner working of peril/danger I don't have lot of confidence in this change (tests and type-check do pass at least locally tho) and before opening speculative PR with it I wanted to check with you what would be proper approach to fix it

orta commented 4 years ago

Yeah, this looks reasonable to me - given that you are running a heroku install, you can also use process.env inside the dangerfiles as well to avoid the Peril object.

The value of the peril obj is when running peril's dangerfiles on separate lambda functions.

pieh commented 4 years ago

given that you are running a heroku install, you can also use process.env inside the dangerfiles as well to avoid the Peril object.

Nice to have workaround like that. I still would prefer peril.env.* approach if fix is feasible - if nothing else, if there are any internal changes to setup of inline runners in the future (i.e. not pass every env var that is available to main process and only pass filtered env vars), we would be back to similar situation in the future.

I did open PR from the diff I linked in issue description and marked actual relevant part ( https://github.com/danger/peril/pull/475 ).