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: Add secret to all webhook's payload where it has been missing #5199

Merged
merged 9 commits into from
Oct 28, 2018
Merged

fix: Add secret to all webhook's payload where it has been missing #5199

merged 9 commits into from
Oct 28, 2018

Conversation

HoffmannP
Copy link
Contributor

affects webhooks for:

  • Delete
  • Fork
  • IssueComment
  • Release

This PR is paired with go-gitea/go-sdk#126

Resolves: #4732, #5173
Signed-off-by: Berengar W. Lehr [email protected]

affects webhooks for:
* Delete
* Fork
* IssueComment
* Release

Resolves: #4732, #5173
Signed-off-by: Berengar W. Lehr <[email protected]>
@bkcsoft bkcsoft added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 27, 2018
@lunny lunny added the type/bug label Oct 27, 2018
@lunny lunny added this to the 1.7.0 milestone Oct 27, 2018
@lunny
Copy link
Member

lunny commented Oct 27, 2018

@appleboy maybe you can review go-gitea/go-sdk#126

@appleboy
Copy link
Member

@lunny Approve and merged.

@sapk
Copy link
Member

sapk commented Oct 27, 2018

@HoffmannP to update vendor please use the dep tool.

dep ensure -update code.gitea.io/sdk

@HoffmannP
Copy link
Contributor Author

@sapk will do next time. Is this advice something to be added to the contribution guideline?

@lafriks
Copy link
Member

lafriks commented Oct 28, 2018

@HoffmannP I think it was somewhere in guidelines. CI will not build otherwise so while it is not updated with go dep PR can not be merged

@sapk
Copy link
Member

sapk commented Oct 28, 2018

The vendoring section say that we use dep but not how to used it.
https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#vendoring

For a quick setup:

#Download the tool
go get -u github.com/golang/dep/cmd/dep
#Update the dep (need $GOPATH/bin in the $PATH)
dep ensure -update code.gitea.io/sdk
#Add Gopkg.toml Gopkg.lock vendor/
git add Gopkg.toml Gopkg.lock vendor/
#And you should be ready to commit the change and push it to the PR.

Berengar W. Lehr added 5 commits October 28, 2018 13:30
* Updated dependency manager via `dep ensure -update code.gitea.io/sdk`
* Gopkg.toml was nat changed as sdk version is set to "master"

Signed-off-by: Berengar W. Lehr <[email protected]>
…P/gitea into issue/#4732_webhooks_add_secret
* WTF just happened?!

Signed-off-by: Berengar W. Lehr <[email protected]>
* Don't know anymore why stuff I never touched is changing

Signed-off-by: Berengar W. Lehr <[email protected]>
@HoffmannP
Copy link
Contributor Author

@sapk I have no idea what I did wrong but files are changing that I never touched and the build is failing at places I never saw before

@sapk
Copy link
Member

sapk commented Oct 28, 2018

@HoffmannP you did nothing wrong.
You update the sdk to the lastest that include your change but also changes from others PR to sdk that were not yet merge in gitea (but validated in sdk).
The error of the CI is because you PR (or an other one to sdk) modify the response of some API call so the definition of the api (swagger file) need to be re-generated.
You should run:

make generate-swagger

This will update the file templates/swagger/v1_json.tmpl that need to be committed. Sorry for not thinking of it before. The CI tests are there to remind us of that 😄.

@HoffmannP
Copy link
Contributor Author

All right than. Getting this done teaches me way more than I could've hoped for. Thank you for the explanation!

@HoffmannP HoffmannP mentioned this pull request Oct 28, 2018
@HoffmannP
Copy link
Contributor Author

Shouldn't #4960 be merged first as it explicitely references the changes to sdk in question? Leaving separate changes in different commits?

Berengar W. Lehr added 2 commits October 28, 2018 22:43
    * Updated dependency manager via `dep ensure -update code.gitea.io/sdk`
    * Gopkg.toml was nat changed as sdk version is set to "master"
    * Rebuild swagger as it became necessary b/c PR go-gitea/go-sdk#121

Signed-off-by: Berengar W. Lehr <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #5199 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5199      +/-   ##
==========================================
+ Coverage    37.5%   37.51%   +<.01%     
==========================================
  Files         309      309              
  Lines       45800    45800              
==========================================
+ Hits        17178    17180       +2     
+ Misses      26160    26158       -2     
  Partials     2462     2462
Impacted Files Coverage Δ
models/repo_list.go 63.37% <0%> (+1.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48badd5...9f0c995. Read the comment docs.

@sapk
Copy link
Member

sapk commented Oct 28, 2018

@HoffmannP It shouldn't be blocking as the fields will just be ignored or empty. The sdk will just be already updated for other PR.

@bkcsoft bkcsoft removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 28, 2018
@bkcsoft bkcsoft added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Oct 28, 2018
@techknowlogick techknowlogick merged commit fb14458 into go-gitea:master Oct 28, 2018
@techknowlogick
Copy link
Member

@HoffmannP would you be able to send a backport PR to release/v1.6 branch ?

@HoffmannP HoffmannP deleted the issue/#4732_webhooks_add_secret branch October 28, 2018 22:19
@HoffmannP
Copy link
Contributor Author

@techknowlogick done

@lafriks lafriks added the backport/done All backports for this PR have been created label Oct 30, 2018
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webhook for "Issue Comment" contains no secret
8 participants