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 LockBranch and AllowForkSyncing to repos.go #2577

Merged
merged 5 commits into from
Nov 28, 2022

Conversation

XaurDesu
Copy link
Contributor

Fixes #2574 , and it is mostly a direct, if barebones, implementation of the plan ideated there. Adding references to LockBranch and AllowForkSyncing. References to RequireLastPushApproval were added in cb24cab . There might be some changes left to do.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

After making the requested changes below, please run gofmt on repos.go and then also run go generate ./... at the top of the repo and push (not force-push) any changes that were made, as described in the file CONTRIBUTING.md.
Thanks.

github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
@XaurDesu
Copy link
Contributor Author

I can't seem to replicate the failing test in my local environment, already ran gofmt and go generate, but it seems to have no effect on tests, i think i might have a configuration problem on my end but i can't seem to figure it out.

@dtdao
Copy link

dtdao commented Nov 28, 2022

From checking the workflow file. The tests didnt run. Its failing at the

- name: Ensure go generate produces a zero diff

specifically at git diff --exit-code; portion of it.

edit:
I think i understand is missing and why you are failing the "tests"

@XaurDesu
So after adding the 2 fields to your struct.

  1. gofmt github.com/repos.go
  2. go generate -x ./...
    With the -x you should see 2 commands being log into your console.
   go run gen-accessors.go
   go run gen-stringify-tests.go
  1. Your git diff should be 3 files now.
  2. Commit those and push and you should resolve the test errors.

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 28, 2022

Yes, you are correct, @dtdao - thank you for the detailed explanation.

This information should also be covered in our CONTRIBUTING.md guide at the top of this repo... if you feel that it can be made clearer, we welcome PRs for it as well.

@XaurDesu
Copy link
Contributor Author

I agree that CONTRIBUTING.md could (and probably should) be clearer about some things, but as it should be evident by now, I'm pretty new to this kind of workflow, and i would rather solve a few more issues to get accustomed to the codebase before messing with any documentation we have, especially kind that regards new contributors. Maybe @dtdao could check that out.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @XaurDesu !
LGTM.
Merging.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #2577 (bc17062) into master (621c4ba) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2577   +/-   ##
=======================================
  Coverage   97.98%   97.98%           
=======================================
  Files         125      125           
  Lines       10875    10875           
=======================================
  Hits        10656    10656           
  Misses        150      150           
  Partials       69       69           
Impacted Files Coverage Δ
github/repos.go 98.67% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

Add new Branch Protection "Require Last Push Approval"
3 participants