Provide default values for most configuration options

[kevindigo]

Rather than always requiring users to create environment variables for everything, I would propose:

• CSV_COLUMN_NUMBER_FOR_GITHUB_ID could default to 0
• CSV_COLUMN_NUMBER_FOR_ALTERNATE_ID could default to 0
• TIMEZONE could default to UTC
• GITHUB_IMPORTANT_EVENTS could default to some reasonable list

This would make it easier for users to try running starfish the first time. It would also avoid the need for tests to set up a bunch of environment variables that aren't directly relevant to the tests themselves.

[kevindigo]

I should make it clear that this is a personal idea I had this morning. Project leadership will have to discuss whether it would really be a good move.

[danisyellis]

I like this idea and think we should do it :-)

Here's some details for implementation: Currently, some of these variables have defaults in the .template.env However, this issue would be changing the code in index.js to contain the defaults instead. For each variable (except GITHUB_TOKEN which will not have a default), the code would check to see whether or not that variable exists in the .env file. If it does, the code will use that value. If not, it will use a default value that will be hardcoded.

In addition to the code change in index.js, the README should be updated to reflect this change (the fact that a user should only copy a given variable from .env.template to their .env if they want to provide their own value. Also, I'm thinking the values in the .env.template should be those same defaults that will be in index.js, but I'm not sure about that?

[kevindigo]

If .env.template allows comments, the variables with defaults could be commented out, with their default values. If not, then I agree that just having them be the same as the code defaults makes sense.

[SagnikH]

Hi, this is my first time contributing to OS, and I would like to give this issue a try. Thank You

[danisyellis]

Hi @SagnikH. That would be great, thank you! Let us know if you have any questions or want us to look at a Work-In-Progress PR or anything like that.

Often, the first step is to get your dev environment set up (get Starfish running for you locally). The README has a ton of info on how to do that.

You've probably already read this, but in case you missed it, we also have a Contributing.MD you should read through.

Thanks again.

[SagnikH]

Hi @danisyellis, I have gone through both of them. I looked at your implementation detail. There you mentioned making changes to the index.js file. I found a globals.js file which is being used to validate & get the env variables & export them. So I was wondering whether we could implement this default behavior inside that file, as we are using that file solely for handling environment variables.

[danisyellis]

@SagnikH Yes, thank you for noticing that! I've done a major refactor since writing those implementation notes, and now all of the globals are being created in globals.js instead of index.js and I didn't think to update this issue.

[SagnikH]

@danisyellis After we make this change, I think we need to change the function name along with some tests in the starfish.test.js file. I found it has a test for, checking if the configuration variable isn't present in our environment variables and test for checking if the value matched with corresponding environment variable. As now we are providing defaults, I think we should change it accordingly, or just remove it altogether.

[danisyellis]

Hi @SagnikH If the function you're talking about is getOrThrowIfMissingOrEmpty, we'll need to keep it to use for githubToken since that env variable can't have a default. But for the other variables, you can either a) create a new function or b) not use a function at all, whichever makes more sense.

In starfish.test.js, you're right that that test won't be necessary for the variables that will have defaults now. So let's change TIMEZONE to 'GITHUB_TOKEN'.

[SagnikH]

Hey @danisyellis, you were right. I have kept the getOrThrowIfMissingOrEmpty function and added another function getDefaultEnvironemntValues to get the default config values. I modified the test like you told as well and added a note in the README file for our users to be aware of the default implementation we have provided for them. I am done & have issued a pull request. In case of any problem with the same do tell & I will gladly try to fix it. Thank You