Skip to content

Conversation

@sevbch
Copy link
Contributor

@sevbch sevbch commented Feb 18, 2025

Context

Today, the GGShield binaries for Linux / Windows MacOS are shipped within the extension.
This is making the extension quite big, and the repo bigger and bigger each time a new version of GGShield is released. (the .git is getting bigger as diffs on binaries aren't efficient)

What has been done

We now fetch the latest GGShield binary by calling GitHub API

  • only the relevant binary is installed, depending on the user distribution
  • we automatically fetch the latest available version. Thus, the extension is automatically updated using latest GGShield version. It means that we won't need anymore to release a new extension version on each new release of GGShield.

👀 Easier to review commit by commit

  • remove GGShield binaries
  • code to install binary from GitHub API
  • fixing a test by properly mocking getGGShieldAbsolutePath
  • adding new tests

Validation

  • checkout this branch
  • run vsce package
  • (uninstall GitGuardian extension if you already have it. Reload your window)
    image
  • manually install the extension using this menu
    image
  • reload your window just in case
  • you should be able to authenticate and the extension should start detecting secrets

PR check list

  • As much as possible, the changes include tests
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry.

@sevbch sevbch changed the title rm ggshield binaries Fetch GGShield binary from GitHub API Feb 19, 2025
@sevbch sevbch force-pushed the severine/download-ggshield-binary branch 2 times, most recently from 841ab1c to ed8e779 Compare February 20, 2025 14:43
@sevbch sevbch self-assigned this Feb 20, 2025
@sevbch sevbch marked this pull request as ready for review February 20, 2025 15:02
@sevbch sevbch requested a review from a team as a code owner February 20, 2025 15:02
@sevbch sevbch force-pushed the severine/download-ggshield-binary branch from 64ace7f to d6f7f5e Compare February 20, 2025 17:25
fix(test): mock getGGShieldAbsolutePath in getConfiguration test

feat: review: send messages to channel output
@sevbch sevbch force-pushed the severine/download-ggshield-binary branch from d6f7f5e to 5db5a14 Compare February 20, 2025 17:29
Copy link
Contributor

@gg-jonathangriffe gg-jonathangriffe left a comment

Choose a reason for hiding this comment

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

Thanks for the MR ! This will be a big improvement.

Would it be possible to add a test over the whole getGGShieldAbsolutePath, to test the case where we already have the correct ggshield, no ggshield and an old ggshield ?

@sevbch
Copy link
Contributor Author

sevbch commented Feb 21, 2025

As I had the conversation with several people, I'm writing down here the rationale about the choice I made here to natively auto-update the extension to the latest GGShield release.

There are 3 main options to me:

  1. The GGShield version is pinned in the code (basically replace this line with sth like ggshieldVersion = 1.36.0).
  2. The GGShield version can be set from an extension parameter.
  3. The extension is auto-updated to latest GGShield version (what I did here).

Let's dive into what would be the process, pros and cons for each option.

Option 1: The GGShield version is pinned in the code

Process: each time a new GGShield version is released, we release a new extension version to upgrade to the new GGShield version.
Pros:

  • If for some reason, a breaking change is introduced in GGShield, we can address it when bumping the extension to latest GGShield version.
  • Moreover, even if we didn't anticipate it, it should be caught ahead of the extension release thanks to failing tests on the PR making the version bump.

Cons:

  • The process is quite cumbersome and maybe too much as it's not often that we have breaking changes in GGShield.

Option 2: The GGShield version can be set from an extension parameter

This is basically option 1, but we authorize to override it with an extension parameter.
Process: same as option 1
Pros: same as option 1
Cons:

  • Same as option 1
  • We would need to ensure that the extension is retro-compatible with previous versions of GGShield. For me, that puts this option completely out, as it's a potential maintenance nightmare.

Option 3: The extension natively auto-update to latest GGShield version

Process:

  • Nothing to do for usual GGShield release
  • If a breaking change is introduced in GGShield
    i. Ahead of GGShield release, make an extension release to pin GGShield version to previous tag.
    ii. After GGShield release, make the required changes in the extension, set back the version to latest tag and release the extension.

Note: step i. & ii. can be merged.

Pros: There is usually nothing to do to maintain the extension up-to-date.

Cons:

  • The breaking change on GGShield case which requires anticipation.
  • Moreover, if the change isn't anticipated, we might face a prod issue on the extension. This is a true risk, but to me it's easily mitigated with a hotfix release applying step i. from described process above. It's just changing 1 line so very quick to implement. The VSCode outage would be very short. Also an extension outage isn't too much impactful for the developers as it doesn't block them in their work.

This is why I chose to go for option 3. Happy to discuss if you think I missed sth!

@sevbch
Copy link
Contributor Author

sevbch commented Feb 21, 2025

Thanks for the MR ! This will be a big improvement.

Would it be possible to add a test over the whole getGGShieldAbsolutePath, to test the case where we already have the correct ggshield, no ggshield and an old ggshield ?

I totally agree with you and I actually spent some time trying to implement a test, but I struggled a lot with the simple-mock lib. It didn't succeed in mocking the inner calls made inside the function (in particular installGGShield).

From my understanding, mocking isn't really part of the TypeScript world, or it seems to be but in a different way (with the stubs / spies logic). From what I found jest could be the relevant lib to use for testing and using stubs, but it implies to refacto the current test suite so I'm not willing to go down that path.

So in conclusion: I think I'm going to add a test which will do the full integration test: fully install the binary when it doesn't exist etc. Not mocks at all. It will be a bit long but at least that should work.
Also, I can add a Linux / Windows / MacOS matrix in the CI tests and thus we'll have a full coverage of this.

@sevbch sevbch force-pushed the severine/download-ggshield-binary branch from d313d4e to 175f722 Compare February 21, 2025 15:14
@sevbch sevbch force-pushed the severine/download-ggshield-binary branch 7 times, most recently from 0313e66 to c39dd4d Compare February 21, 2025 16:36
@sevbch sevbch force-pushed the severine/download-ggshield-binary branch from c39dd4d to cbeb0fb Compare February 21, 2025 16:40
@sevbch
Copy link
Contributor Author

sevbch commented Feb 21, 2025

@gg-jonathangriffe see the 3 last commits for review

@sevbch sevbch force-pushed the severine/download-ggshield-binary branch from 4bda9cc to 54a5866 Compare February 24, 2025 10:13
@sevbch sevbch force-pushed the severine/download-ggshield-binary branch 2 times, most recently from df49c2b to d33262c Compare February 24, 2025 13:20
@sevbch
Copy link
Contributor Author

sevbch commented Feb 24, 2025

As I had the conversation with several people, I'm writing down here the rationale about the choice I made here to natively auto-update the extension to the latest GGShield release.

There are 3 main options to me:

  1. The GGShield version is pinned in the code (basically replace this line with sth like ggshieldVersion = 1.36.0).
  2. The GGShield version can be set from an extension parameter.
  3. The extension is auto-updated to latest GGShield version (what I did here).

Let's dive into what would be the process, pros and cons for each option.

Option 1: The GGShield version is pinned in the code

Process: each time a new GGShield version is released, we release a new extension version to upgrade to the new GGShield version. Pros:

  • If for some reason, a breaking change is introduced in GGShield, we can address it when bumping the extension to latest GGShield version.
  • Moreover, even if we didn't anticipate it, it should be caught ahead of the extension release thanks to failing tests on the PR making the version bump.

Cons:

  • The process is quite cumbersome and maybe too much as it's not often that we have breaking changes in GGShield.

Option 2: The GGShield version can be set from an extension parameter

This is basically option 1, but we authorize to override it with an extension parameter. Process: same as option 1 Pros: same as option 1 Cons:

  • Same as option 1
  • We would need to ensure that the extension is retro-compatible with previous versions of GGShield. For me, that puts this option completely out, as it's a potential maintenance nightmare.

Option 3: The extension natively auto-update to latest GGShield version

Process:

  • Nothing to do for usual GGShield release
  • If a breaking change is introduced in GGShield
    i. Ahead of GGShield release, make an extension release to pin GGShield version to previous tag.
    ii. After GGShield release, make the required changes in the extension, set back the version to latest tag and release the extension.

Note: step i. & ii. can be merged.

Pros: There is usually nothing to do to maintain the extension up-to-date.

Cons:

  • The breaking change on GGShield case which requires anticipation.
  • Moreover, if the change isn't anticipated, we might face a prod issue on the extension. This is a true risk, but to me it's easily mitigated with a hotfix release applying step i. from described process above. It's just changing 1 line so very quick to implement. The VSCode outage would be very short. Also an extension outage isn't too much impactful for the developers as it doesn't block them in their work.

This is why I chose to go for option 3. Happy to discuss if you think I missed sth!

Finally took option 1 here after discussion with @agateau-gg

@sevbch sevbch force-pushed the severine/download-ggshield-binary branch 2 times, most recently from e65a42a to 65eb793 Compare February 24, 2025 13:48
@sevbch sevbch force-pushed the severine/download-ggshield-binary branch from 65eb793 to 1caf412 Compare February 24, 2025 13:52
@sevbch sevbch requested a review from agateau-gg February 24, 2025 13:54
Copy link
Collaborator

@agateau-gg agateau-gg left a comment

Choose a reason for hiding this comment

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

Minor remark, approving anyway.

@sevbch sevbch merged commit e99b365 into main Feb 27, 2025
3 checks passed
@sevbch sevbch deleted the severine/download-ggshield-binary branch February 27, 2025 07:41
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.

4 participants