feat: Add Bitbucket retriever #2611
Conversation
✅ Deploy Preview for go-feature-flag-doc-preview canceled.
|
There was a problem hiding this comment.
Hey @CollinsC1O thanks a lot for this pull request.
We are going to the right direction, I have made a few suggestion and pin point some issue.
I let you review them, and let me know if you need any help.
README.md
Outdated
| @@ -19,9 +19,9 @@ | |||
| <a href="https://github.com/sponsors/thomaspoignant"><img src="https://img.shields.io/github/sponsors/thomaspoignant?logo=GitHub%20Sponsors" alt="Sponsords"></a> | |||
| </p> | |||
|
|
|||
| > :pray: If you are using **GO Feature Flag** please consider to add yourself in the [adopters](./ADOPTERS.md) list. | |||
There was a problem hiding this comment.
issue (markdown formatting): Removing the end of line whitespaces in the README.md file is causing a change on how the document will look like. Those are used to have a line break, and removing them is not ideal.
suggestion (markdown formatting): Restore those double whitespace in the end of the lines to avoid changing how the readme looks.
cmd/relayproxy/config/retriever.go
Outdated
| RedisOptions *redis.Options `mapstructure:"redisOptions" koanf:"redisOptions"` | ||
| RedisPrefix string `mapstructure:"redisPrefix" koanf:"redisPrefix"` | ||
| Workspace string `mapstructure:"bitbucketWorkspace" koanf:"bitbucketWorkspace"` | ||
| BitbucketToken string `mapstructure:"bitbucketToken" koanf:"bitbucketToken"` |
There was a problem hiding this comment.
suggestion: Instead of adding a new specific field for bitbucket, I suggest that we are using the already existing field AuthToken to set the configuration.
cmd/relayproxy/config/retriever.go
Outdated
| Collection string `mapstructure:"collection" koanf:"collection"` | ||
| RedisOptions *redis.Options `mapstructure:"redisOptions" koanf:"redisOptions"` | ||
| RedisPrefix string `mapstructure:"redisPrefix" koanf:"redisPrefix"` | ||
| Workspace string `mapstructure:"bitbucketWorkspace" koanf:"bitbucketWorkspace"` |
There was a problem hiding this comment.
suggestion: ✂️ After applying this suggestion, we can remove this configuration from the relay proxy.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2611 +/- ##
=======================================
Coverage ? 84.94%
=======================================
Files ? 102
Lines ? 4649
Branches ? 0
=======================================
Hits ? 3949
Misses ? 559
Partials ? 141 ☔ View full report in Codecov by Sentry. |
|
Hello @thomaspoignant so I just made some changes. |
There was a problem hiding this comment.
Hey @CollinsC1O thanks for your update but it seems that your commit 92818f3 has removed all recent new features that we have introduce recently.
Can you try to check what happened?
My guess is that the resynchronisation of your repo did not work properly.
I also so that you did not addressed the comments about the README.md file #2611 (comment) and about the change in the relay proxy configuration #2611 (comment) and #2611 (comment).
| Branch: func() string { | ||
| if c.Branch == "" { | ||
| return config.DefaultRetriever.GitBranch | ||
| } | ||
| return c.Branch | ||
| }(), |
There was a problem hiding this comment.
suggestion: we can simplify that part because the default value is already handled in the retriever here.
| Branch: func() string { | |
| if c.Branch == "" { | |
| return config.DefaultRetriever.GitBranch | |
| } | |
| return c.Branch | |
| }(), | |
| Branch: c.Branch, |
CollinsC1O
force-pushed
the
bit_bucket_retriever
branch
from
14be276
to
cba1570
Compare
| cNotif.WebhookURL = cNotif.SlackWebhookURL // nolint | ||
| } | ||
| notifiers = append(notifiers, &slacknotifier.Notifier{SlackWebhookURL: cNotif.WebhookURL}) | ||
| notifiers = append(notifiers, &slacknotifier.Notifier{SlackWebhookURL: cNotif.SlackWebhookURL}) |
There was a problem hiding this comment.
🔨 issue: Why did you removed the change on the slack notifier here?
| @@ -332,8 +336,7 @@ func initNotifier(c []config.NotifierConf) ([]notifier.Notifier, error) { | |||
| Headers: cNotif.Headers, | |||
| }, | |||
| ) | |||
| case config.DiscordNotifier: | |||
| notifiers = append(notifiers, &discordnotifier.Notifier{DiscordWebhookURL: cNotif.WebhookURL}) | |||
|
|
|||
There was a problem hiding this comment.
🔨 issue: Why did you removed the discord integration? Is this on purpose?
There was a problem hiding this comment.
it wasn't on purpose. I must have made this mistakes in a bit of resolving conflict
|
No description provided.