Last known issue with the stash/bitbucket server remote. Will now not… #1928
Conversation
… disable and override other hooks. It’ll add to the list of hooks and remove from the list of hooks on the project up to the max of 20 hooks
| @@ -214,3 +250,142 @@ func (c *Client) paginatedRepos(start int) ([]*Repo, error) { | |||
| } | |||
| return repoResponse.Values, nil | |||
| } | |||
|
|
|||
| func Filter(vs []string, f func(string) bool) []string { | |||
There was a problem hiding this comment.
this should be private (ie lowercase)
| } | ||
|
|
||
| //TODO: find a clean way of doing these next two methods- bitbucket server hooks only support 20 cb hooks | ||
| func arrayToHookSettings(hooks []string) HookSettings { |
There was a problem hiding this comment.
Can you provide a bit of context? The code looks a bit unorthodox, but I get the sense that this is because the Stash API is not user-friendly. So I think the code is probably fine to merge (thanks!) but I just want to understand a bit better.
There was a problem hiding this comment.
It's part of the api for updating the hooks. I wish it was better :/ I imagine there's a way to not have to list out every struct property in goLang and make it a bit less verbose(probably using introspection), I just haven't had a chance to look at it yet.
There was a problem hiding this comment.
yea, I figured it had something to do with the stash api being a bit odd. I see nothing wrong with the current approach, since Go reflection might reduce the amount of code, but at the cost of readability.
another option could be just to unmarshal to a map[string]string so you can build the key dynamically with map["hook-url-"+i]=..., but again, not sure this is really necessary. I sort of like that your implementation sort of implements the Stash API verbatim, and doesn't try to make it seem more simple or hide complexity.
There was a problem hiding this comment.
yeah, that's the only way I could think of doing it otherwise the readability would take a hit which is why I did it this way, until I could think of a "simpler" way. Do you need me to remove the comments?
|
@bradrydzewski Just wanted to see if you needed anything else on this one? :) |
| @@ -12,18 +12,20 @@ import ( | |||
| log "github.com/Sirupsen/logrus" | |||
| "github.com/drone/drone/model" | |||
| "github.com/mrjones/oauth" | |||
| "strings" | |||
There was a problem hiding this comment.
the strings package should be grouped with the other standard library packages. This is a recommended convention for Go projects.
import (
"bytes"
"encoding/json"
"fmt"
"io"
"io/ioutil"
"net/http"
"strconv"
+ "strings"
log "github.com/Sirupsen/logrus"
"github.com/drone/drone/model"
"github.com/mrjones/oauth"
- "strings"
)
|
I added a comment, but in retrospect, I don't want to hold up this pull request for a style change related to package ordering :) I'll update the package ordering next time I'm in this part of the code. |
|
@bradrydzewski thanks for that! if you don't get to it before me, next time I'm in there I'll re-arrange. |
… disable and override other hooks. It’ll add to the list of hooks and remove from the list of hooks on the project up to the max of 20 hooks
This should be the last "known" issue with the remote :)