Skip to content

consolidate and streamline contribution docs#494

Merged
nodo merged 2 commits into
onsi:masterfrom
grosser:grosser/contrib
Jul 13, 2018
Merged

consolidate and streamline contribution docs#494
nodo merged 2 commits into
onsi:masterfrom
grosser:grosser/contrib

Conversation

@grosser

@grosser grosser commented Jun 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 grosser force-pushed the grosser/contrib branch from 7de3a5f to b6a50ac Compare July 1, 2018 15:10
@grosser

grosser commented Jul 4, 2018

Copy link
Copy Markdown
Contributor Author

@AlexeySoshin @onsi does this look good to you ?

nodo
nodo requested changes Jul 4, 2018
View reviewed changes

@nodo nodo left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread CONTRIBUTING.md Outdated

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

@nodo nodo Jul 2, 2018

Copy link
Copy Markdown
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 "...")

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

@nodo nodo Jul 4, 2018

Copy link
Copy Markdown
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

Comment thread CONTRIBUTING.md

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

@nodo nodo Jul 4, 2018

Copy link
Copy Markdown
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

nodo commented Jul 4, 2018

Copy link
Copy Markdown
Collaborator

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

@grosser grosser force-pushed the grosser/contrib branch from b6a50ac to 1efc3d7 Compare July 4, 2018 22:52
@grosser

grosser commented Jul 4, 2018

Copy link
Copy Markdown
Contributor Author

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

grosser commented Jul 4, 2018

Copy link
Copy Markdown
Contributor Author

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
Comment thread CONTRIBUTING.md Outdated
Fork the repo, then:

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

@nodo nodo Jul 12, 2018

Copy link
Copy Markdown
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

Comment thread CONTRIBUTING.md
```

## Making the PR
- make your changes

@nodo nodo Jul 12, 2018

Copy link
Copy Markdown
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

nodo commented Jul 12, 2018

Copy link
Copy Markdown
Collaborator

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

nodo commented Jul 12, 2018

Copy link
Copy Markdown
Collaborator

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

grosser commented Jul 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@nodo

nodo commented Jul 13, 2018

Copy link
Copy Markdown
Collaborator

LGTM, thanks @grosser ! 🎉

@nodo nodo merged commit d848015 into onsi:master Jul 13, 2018
@grosser

grosser commented Jul 13, 2018

Copy link
Copy Markdown
Contributor Author

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.

2 participants

@grosser @nodo