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

Add Gitlab retriever #696

Merged
merged 7 commits into from
Apr 28, 2023
Merged

Conversation

ruairi-wmf
Copy link
Contributor

Description

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking changes (change that is not backward-compatible and/or changes current functionality)

Closes issue(s)

Resolve #695

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the documentation (README.md and /website/docs)
  • I have followed the contributing guide

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for go-feature-flag-doc-preview ready!

Name Link
🔨 Latest commit 2189806
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/644bdc80f47d7300082e8e56
😎 Deploy Preview https://deploy-preview-696--go-feature-flag-doc-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #696 (2189806) into main (d0b0c0d) will increase coverage by 0.10%.
The diff coverage is 93.84%.

@@            Coverage Diff             @@
##             main     #696      +/-   ##
==========================================
+ Coverage   88.94%   89.04%   +0.10%     
==========================================
  Files          58       59       +1     
  Lines        2713     2775      +62     
==========================================
+ Hits         2413     2471      +58     
- Misses        245      248       +3     
- Partials       55       56       +1     
Impacted Files Coverage Δ
cmd/relayproxy/config/config.go 94.39% <ø> (ø)
cmd/relayproxy/service/gofeatureflag.go 56.61% <88.88%> (+1.36%) ⬆️
retriever/gitlabretriever/retriever.go 93.75% <93.75%> (ø)
cmd/relayproxy/config/retriever.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Hello @ruairi-wmf thanks a lot for your pull request this is an awesome addition to GO Feature Flag.

I have looked at your PR and I have a few things that are not working.
I saw that you are not using the GitLab API to retrieve the files but the public raw access from the website.

From my early investigation, I think it could be great to use the API instead.
This API can help us doing what we want to do: https://docs.gitlab.com/ee/api/repository_files.html#get-raw-file-from-repository

I've done a quick CURL to test it and it seems to work perfectly.

curl --header "PRIVATE-TOKEN: <GITLAB TOKEN>" "https://gitlab.com/api/v4/projects/<REPOSITORY SLUG url encoded>/repository/files/<PATH OF YOUR file url encoded>/raw?ref=main"

A point of attention is to be sure that you URL encode both the repository slug and the path of your file.

  • If your slug is thomas.poignant/test-go-feature-flag-provider is should be thomas.poignant%2Ftest-go-feature-flag-provider in the URL.
  • If your path is testdata/flags.yaml it should be testdata%2Fflags.yaml in the URL.

retriever/gitlabretriever/retriever.go Outdated Show resolved Hide resolved
retriever/gitlabretriever/retriever.go Outdated Show resolved Hide resolved
cmd/relayproxy/config/retriever.go Outdated Show resolved Hide resolved
retriever/gitlabretriever/retriever.go Outdated Show resolved Hide resolved
@ruairi-wmf
Copy link
Contributor Author

Thanks Thomas, I'll work on this tonight. Cheers!

Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Hey thanks again for the changes, I have left some new comments.
If you need any help or more information let me know.

cmd/relayproxy/service/gofeatureflag_test.go Show resolved Hide resolved
retriever/gitlabretriever/retriever.go Show resolved Hide resolved
retriever/gitlabretriever/retriever.go Outdated Show resolved Hide resolved
retriever/gitlabretriever/retriever.go Outdated Show resolved Hide resolved
retriever/gitlabretriever/retriever_test.go Outdated Show resolved Hide resolved
retriever/gitlabretriever/retriever.go Outdated Show resolved Hide resolved
retriever/gitlabretriever/retriever.go Outdated Show resolved Hide resolved
@thomaspoignant
Copy link
Owner

Hey @ruairi-wmf I have used your PR to finish it with my remarks.
I will merge it Today and create a new version 1.10.0, you will be able to use the Gitlab retriever starting this version.

Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@kodiakhq kodiakhq bot merged commit 9679c97 into thomaspoignant:main Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Gitlab Retriever) Add logic to allow gitlab retriever and non-public Git Repos
2 participants