Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

feat: add version-checker to ganache startup #3285

Open
wants to merge 220 commits into
base: develop
Choose a base branch
from

Conversation

tenthirtyone
Copy link
Contributor

Description

This is the follow-on to #2859. The majority of code has already been reviewed and was merged when I slammed the green button home (wewps). The big change here is that the default config is set to opt-out so users will have to opt-in.

It might make sense to add a command line switch or easier way to toggle to opt-in. But once it's baked for a few weeks with the team opting in and any issues have been caught/corrected we'll slip the switch to opt-in by default.

davidmurdoch and others added 30 commits April 6, 2022 19:23
)

No change in behaviour, but makes working with local forks ever so slightly faster.
* add vX.x.x tag on release

* merge master into develop after successful publish

* add action to import TrufBot GPG keys for signed commits
Copy link
Contributor

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

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

Partial review

src/packages/cli/src/cli.ts Show resolved Hide resolved
src/packages/version-check/src/semver.ts Show resolved Hide resolved
src/packages/version-check/tests/version-check.test.ts Outdated Show resolved Hide resolved
@MicaiahReid MicaiahReid self-requested a review January 12, 2023 15:30
@@ -14,7 +14,7 @@ import type { VersionCheckOptions } from "./types";
// Why is this not part of the config? Well, if a user
// set this to a low value there could be problems.
const ONE_DAY: number = 86400;

const ACTIVATED = process.env.VC_ACTIVATED || false;
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 prefer if the behaviour around process.env.VS_ACTIVATED was more obvious (ie the type of ACTIVATED is string | false).

Maybe something like

Suggested change
const ACTIVATED = process.env.VC_ACTIVATED || false;
const isActivated = process.env.VC_ACTIVATED === "true";

// This file will eventually be replaced by another project. For now, we want to write to the
// future location in the namespace for VersionCheck.
// https://github.com/trufflesuite/ganache/pull/3285/files/0644054b8458eafdb52f2a9699842ed0c91f6a4e#r1068731549
// Ive seen this crash when '/' is in the projectName, e.g. "Ganache/cli" but it seems ok if a file exists already.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to investigate this crash further?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

7 participants