Throw error when requiredEnvVars are missing

[swinton]

👋 @JasonEtco thanks for providing this utility, it's super helpful!

What are your thoughts on throwing an error (saying, e.g. GitHub Actions environment unavailable) when the required environment variables are missing? My thoughts are this would make it a little more explicit when the Actions environment isn't set, and would allow me to decide how to proceed (should I abort, or try and do something useful).

actions-toolkit/src/index.ts

Lines 142 to 167 in eecccc3

	private warnForMissingEnvVars () {
	const requiredEnvVars = [
	'HOME',
	'GITHUB_WORKFLOW',
	'GITHUB_ACTION',
	'GITHUB_ACTOR',
	'GITHUB_REPOSITORY',
	'GITHUB_EVENT_NAME',
	'GITHUB_EVENT_PATH',
	'GITHUB_WORKSPACE',
	'GITHUB_SHA',
	'GITHUB_REF',
	'GITHUB_TOKEN'
	]
	const requiredButMissing = requiredEnvVars.filter(key => !process.env.hasOwnProperty(key))
	if (requiredButMissing.length > 0) {
	// This isn't being run inside of a GitHub Action environment!
	const list = requiredButMissing.map(key => `- ${key}`).join('\n')
	const warning = `There are environment variables missing from this runtime, but would be present on GitHub.\n${list}`
	// tslint:disable-next-line:no-console
	console.warn(warning)
	this.warning = warning
	}
	}

[JasonEtco]

Hi @swinton! Thank you for opening this - I really grappled with this decision. I'm happy to talk through it and properly throw (and even go so far as to process.exit(1)) - so here's my reasoning for how it is now, let me know what you think.

I had it warn rather than throw because of tests. I didn't want users of the library to always need to define every one of those env vars just to make their tests pass. Also, its for running actions code locally - I can run my code as a Node process without having to set all those env vars (which would be cumbersome as inline CLI arguments, or you'd need dotenv).

Definitely open to opinions here - I'm not 100% convinced either way honestly.

[swinton]

Thanks for taking a look here 🙇

I had it warn rather than throw because of tests. I didn't want users of the library to always need to define every one of those env vars just to make their tests pass.

Yeah, interesting point. I might be weird, but I typically mock out Toolkit in my tests to emulate the expected Actions environment with something like this (which allows me to test various error paths in my code):

const tools = {
  context: {
    event: 'issues'
  }
};
Toolkit.mockImplementation(() => {
  return tools;
});

Also, its for running actions code locally - I can run my code as a Node process without having to set all those env vars (which would be cumbersome as inline CLI arguments, or you'd need dotenv).

Interesting point. For what it's worth, when running locally I typically send in environment variables with docker run -e, i.e. in a simple run.sh I have:

#!/bin/sh
docker run \
  -e HOME=/root \
  -e GITHUB_WORKFLOW=main \
  -e GITHUB_ACTION=my-github-action \
  -e GITHUB_ACTOR=octocat \
  -e GITHUB_REPOSITORY=swinton/example \
  -e GITHUB_EVENT_NAME=issues \
  -e GITHUB_EVENT_PATH=/test/fixtures/issues.closed.json \
  -e GITHUB_WORKSPACE=/github/workspace \
  -e GITHUB_SHA=0000000 \
  -e GITHUB_REF=refs/heads/master \
  -e GITHUB_TOKEN=$GITHUB_TOKEN \
  my-github-action

I know there's also act, I just haven't had a chance to use that yet.

Definitely open to opinions here - I'm not 100% convinced either way honestly.

So, my slight preference is to throw when the environment is not setup as expected, as it sends a clearer signal for what went wrong / what I likely overlooked in my implementation (oops, I forgot to send in the environment variables, better whip up a script... kinda thing).

Currently I'm working around this with something like:

const getTools = () => {
  const tools = new Toolkit();
  if (typeof tools.context.event === 'undefined') {
    throw new Error('GitHub Actions environment unavailable');
  }
  return tools;
};

But relying ontools.context.event being undefined feels a little brittle 🤔

[JasonEtco]

Iiiiiiinteresting. Thanks for the clear description 🙏 The reliability of knowing that your implementation is correct makes a lot of sense.

Interesting point. For what it's worth, when running locally I typically send in environment variables with docker run -e, i.e. in a simple run.sh I have:

I might steal this and put it into the bootstrap script - it looks really handy and would avoid a "why isn't my thing working oh its because I'm missing this env var."

I'm convinced! Will make it throw 👍 possibly with an option to just warn in the constructor.

[JasonEtco]

@swinton 👋 I'm here to report that I reverted #41 in #70 😅 I discovered that some environment variables are not present in every event, so to depend on the existence of some but not all is as unreliable as none (that's confusing but I think it's correct). I also encountered some friction while using this in v2.0.0-beta.1 - this was expected, but still not a great experience.

I'm going to close this issue because after investigating it and trying it out, I don't believe that the predictability is worth the friction, especially because actions-toolkit is a good way to start writing Actions, where newcomers to the platform aren't familiar with the nuances of where the event.json lives, what env vars do what, etc.

[JasonEtco]

Oh by the by @swinton - this may or may not work for your use-case, but you can define what events your Action is designed to be triggered by, which would help prevent undefined events!

[swinton]

Thanks for taking a look @JasonEtco. Makes sense!