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

fix: make API urls from GitGuardian work with GGShield #1045

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

sevbch
Copy link
Contributor

@sevbch sevbch commented Jan 8, 2025

Context

See GitGuardian/gitguardian-vscode#67

What has been done

When running ggshield cmd --instance ${INSTANCE}, if INSTANCE matches any of the URLs mentioned https://api.eu1.gitguardian.com/docs, it is replaced by the relevant URL.

Validation

Validate with GGShield

  • Checkout this branch
  • Run any ggshield command having the --instance option available with --instance https://api.gitguardian.com/v1 or --instance https://api.eu1.gitguardian.com/v1
  • Command should work the same as if you put --instance https://dashboard.gitguardian.com/ or --instance https://dashboard.eu1.gitguardian.com/

Validate with VSCode Extension

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

@sevbch sevbch changed the title fix: makeAPI urls from GitGuardian work with GGShield fix: make API urls from GitGuardian work with GGShield Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.75%. Comparing base (cb558e9) to head (cc27bb4).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1045   +/-   ##
=======================================
  Coverage   91.75%   91.75%           
=======================================
  Files         143      143           
  Lines        6040     6043    +3     
=======================================
+ Hits         5542     5545    +3     
  Misses        498      498           
Flag Coverage Δ
unittests 91.75% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sevbch sevbch force-pushed the severine/handle-product-doc-api-urls branch 2 times, most recently from 759457d to df36e15 Compare January 8, 2025 14:29
@sevbch sevbch marked this pull request as ready for review January 8, 2025 16:16
@sevbch sevbch requested a review from a team as a code owner January 8, 2025 16:16
@sevbch sevbch self-assigned this Jan 8, 2025
@sevbch sevbch force-pushed the severine/handle-product-doc-api-urls branch 3 times, most recently from 198144f to a0734ee Compare January 8, 2025 16:26
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.

Looks good to me ! Thanks for the improvement, I just have a small comment.

ggshield/core/config/config.py Outdated Show resolved Hide resolved
ggshield/core/config/config.py Outdated Show resolved Hide resolved
@sevbch
Copy link
Contributor Author

sevbch commented Jan 10, 2025

to answer you both @gg-jonathangriffe and @agateau-gg:

I think on-prem urls should be handled as well, as the issue encountered by one of GitGuardian customer with the VSCode Extension could happen too.

The API url looks like this for an on-prem customer.

I'm going to adapt this MR and add a test for this use case.

@sevbch
Copy link
Contributor Author

sevbch commented Jan 10, 2025

to answer you both @gg-jonathangriffe and @agateau-gg:

I think on-prem urls should be handled as well, as the issue encountered by one of GitGuardian customer with the VSCode Extension could happen too.

The API url looks like this for an on-prem customer.

I'm going to adapt this MR and add a test for this use case.

See last commit

@sevbch sevbch force-pushed the severine/handle-product-doc-api-urls branch from c8761c1 to c0f69ec Compare January 13, 2025 12:56
@sevbch
Copy link
Contributor Author

sevbch commented Jan 13, 2025

@gg-jonathangriffe & @agateau-gg : I finished manual checks, this PR is ready for re-review

@sevbch sevbch force-pushed the severine/handle-product-doc-api-urls branch 2 times, most recently from ce964f3 to c5e624b Compare January 13, 2025 15:47
@sevbch sevbch force-pushed the severine/handle-product-doc-api-urls branch from c5e624b to cc27bb4 Compare January 13, 2025 15:58
@agateau-gg agateau-gg merged commit 8b6464d into main Jan 14, 2025
32 checks passed
@agateau-gg agateau-gg deleted the severine/handle-product-doc-api-urls branch January 14, 2025 14:05
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.

3 participants