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: use API urls from official doc #67

Closed
wants to merge 1 commit into from

Conversation

sevbch
Copy link
Contributor

@sevbch sevbch commented Dec 30, 2024

Context

API url to use isn't consistent with GitGuardian API documentation. Thus when users try to set API url param to https://api.gitguardian.com/v1 or https://api.eu1.gitguardian.com/v1, login fail.

What has been done

When a url from official documentation is provided, we replace it with its matching one working with GGShield.

Validation

  • checkout this branch
  • launch the extension
  • set API url param to https://api.gitguardian.com/v1 or https://api.eu1.gitguardian.com/v1
  • reload window
  • you should have logged in perfectly

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.
    @salome-voltz @gg-jonathangriffe is there a way to add a changelog entry? Apart from updating CHANGELOG.md in the release PR?

@sevbch sevbch changed the title fix: use urls from official doc fix: use API urls from official doc Dec 30, 2024
@sevbch sevbch force-pushed the severine/fix-default-api-url branch from b40dcc5 to 76b5b45 Compare December 30, 2024 14:20
@sevbch sevbch self-assigned this Dec 30, 2024
@sevbch sevbch force-pushed the severine/fix-default-api-url branch from 76b5b45 to 54627eb Compare December 30, 2024 14:48
@sevbch sevbch force-pushed the severine/fix-default-api-url branch from 54627eb to 03f405e Compare December 30, 2024 14:49
@sevbch sevbch marked this pull request as ready for review December 30, 2024 14:51
@sevbch sevbch requested a review from a team as a code owner December 30, 2024 14:51
@gg-jonathangriffe
Copy link
Contributor

I think that the extension should prioritize being consistent with ggshield more than with the documentation.
In ggshield, settings the URL to https://api.gitguardian.com/v1 does not work, therefore I think we should keep it simple here and not do any additional treatment.
If we want to be more flexible, we should probably do it in ggshield.
WDYT ?

@sevbch
Copy link
Contributor Author

sevbch commented Jan 8, 2025

I think that the extension should prioritize being consistent with ggshield more than with the documentation. In ggshield, settings the URL to https://api.gitguardian.com/v1 does not work, therefore I think we should keep it simple here and not do any additional treatment. If we want to be more flexible, we should probably do it in ggshield. WDYT ?

You're right, I implemented changes in GGShield in GitGuardian/ggshield#1045 . Let me know what you think.

Closing this PR

@sevbch sevbch closed this Jan 8, 2025
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.

2 participants