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.

Sign up for GitHub

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

Jump to bottom

consolidate and streamline contribution docs #494

Merged
merged 2 commits into from
Jul 13, 2018
Merged

consolidate and streamline contribution docs #494

merged 2 commits into from
Jul 13, 2018

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Jun 28, 2018 •
edited

solves #492 (comment)

before:

  • 3 different places for docs
  • no clear way of how to get tests running
  • long sentences with too many words that say very little
  • tests not actually running

after:

  • single place for contribution
  • copy-paste instructions
  • short/clear instructions
  • working tests

@grosser grosser mentioned this pull request Jun 28, 2018
Closed
@grosser
Copy link
Contributor Author

grosser commented Jul 4, 2018

@AlexeySoshin @onsi does this look good to you ?

nodo
nodo requested changes Jul 4, 2018
View reviewed changes
Copy link
Collaborator

@nodo nodo left a comment

Choose a reason for hiding this comment

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

Thanks for helping @grosser!

Please see comments above, I am happy to support in case you have doubts.

CONTRIBUTING.md Outdated

```
git clone git@github.com:<NAME>/ginkgo.git $GO_PATH/src/github.com/<NAME>/ginkgo`
go get github.com/onsi/gomega
Copy link
Collaborator

Choose a reason for hiding this comment

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

you need to install gomega using go get github.com/onsi/gomega/... (see the "...")

CONTRIBUTING.md Outdated
## Making the PR
- make your changes
- run tests and linter again (see above)
- run `/after_pr.sh` to undo modifications
Copy link
Collaborator

Choose a reason for hiding this comment

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

after_pr.sh does not exist anymore I think

CONTRIBUTING.md Outdated

./before_pr.sh # replace imports to test internal packages
ginkgo -r -p # ensure tests are green
go vet ./... # ensure linter is happy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, this does not work from a fork. So, I think we should fix that before merging this PR.

@nodo
Copy link
Collaborator

nodo commented Jul 4, 2018

I have opened #496 to track the bug I am seeing when running the tests in a fork.

@grosser
Copy link
Contributor Author

grosser commented Jul 4, 2018

finally go the tests running, the issue was that extensions/table/table.go had github.com/onsi/ginkgo which needed to be replaced to github.com/grosser/ginkgo ... I changed the before_pr to make that change

... maybe this is just the wrong approach and the fork should be kept under onsi/ginkgo to avoid all this madness ?

@grosser
Copy link
Contributor Author

grosser commented Jul 4, 2018

I'd think the current state is already a step forward, so maybe merge and then do adjustments in future PRs ?

nodo
nodo requested changes Jul 12, 2018
View reviewed changes
CONTRIBUTING.md Outdated
Fork the repo, then:

```
git clone git@github.com:<NAME>/ginkgo.git $GO_PATH/src/github.com/<NAME>/ginkgo`
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a typo $GO_PATH should be $GOPATH

CONTRIBUTING.md Outdated
```

## Making the PR
- make your changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add git commit here? Otherwise git checkout . suggested as later step would revert the changes.

@nodo
Copy link
Collaborator

nodo commented Jul 12, 2018

I agree that's good improvement, thank you for that! I have added a few comments, let me know what you think.

Also, what is the output you are getting when you run the steps? Do the tests pass?

@nodo
Copy link
Collaborator

nodo commented Jul 12, 2018

I had a thought about it. I think that current flow we are suggesting is not right. In my opinion the steps should be:

  1. Fork ginkgo
  2. Clone onsi/ginkgo in your GOPATH, or even better use go get
  3. cd onsi/ginkgo
  4. Add the fork as new remote git remote add fork https://github.com/YOUR_USERNAME/ginkgo
  5. git push fork

Tests should always pass because the imports won't change. What do you think @grosser ?

@nodo nodo mentioned this pull request Jul 12, 2018
Closed
@grosser
Copy link
Contributor Author

grosser commented Jul 13, 2018

updated ... much simpler and less juggling things around when trying to make a PR

@nodo
Copy link
Collaborator

nodo commented Jul 13, 2018

LGTM, thanks @grosser ! 🎉

@nodo nodo merged commit d848015 into onsi:master Jul 13, 2018
@grosser
Copy link
Contributor Author

grosser commented Jul 13, 2018

thx 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nodo nodo nodo requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@grosser @nodo