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

Change syncAllowance default value as an environment variable #158

Merged
merged 3 commits into from
Jul 13, 2021

Conversation

rem1niscence
Copy link
Contributor

@rem1niscence rem1niscence commented Jul 13, 2021

  • Change sync allowance to a environment variable and change the workflows to reflect the default value
  • Change the null coalescing operator as is not supported on the project's node engine

Fixes #156

@rem1niscence rem1niscence requested review from nymd and Evalir July 13, 2021 17:59
@@ -8,6 +8,8 @@ const logger = require('../services/logger')

import axios from 'axios'

const DEFAULT_SYNC_ALLOWANCE: number = parseInt(process.env.DEFAULT_SYNC_ALLOWANCE) || 5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather have all of the .env variables checked in one place and rather than silently fallback to a default, I'd rather it error out and tell you that you made a dumb mistake and forgot the var.

Probably around here:

const aatPlan = process.env.AAT_PLAN || AatPlans.PREMIUM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like having .env vars in one place—this is how I usually do it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does look nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, like it too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now I'm doing a quick fix but an issue can be opened for that to change all the envs

const pocketBlockTime: string = process.env.POCKET_BLOCK_TIME || ''
const relayRetries: string = process.env.POCKET_RELAY_RETRIES || ''
const databaseEncryptionKey: string = process.env.DATABASE_ENCRYPTION_KEY || ''
const defaultSyncAllowance: number = parseInt(process.env.DEFAULT_SYNC_ALLOWANCE) || -1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is 0 a valid value or must always be over 0? @nymd

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 is valid though it would be very restrictive. It should be allowed. But not blank or undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all right, then this is ready

@rem1niscence rem1niscence changed the title Sync allowance hardcode Change syncAllowance default value as an environment variable Jul 13, 2021
@rem1niscence rem1niscence merged commit 9d1f01f into develop Jul 13, 2021
@rem1niscence rem1niscence deleted the sync-allowance-hardcode branch July 13, 2021 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change hardcoding of default sync allowance
3 participants