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 CONTRIBUTING.md #79

Merged
merged 1 commit into from May 18, 2019

Conversation

Projects
None yet
4 participants
@bhavin192
Copy link
Contributor

commented Apr 4, 2019

@bhavin192 bhavin192 requested review from PrasadG193 and mugdha-adhav Apr 4, 2019

@bhavin192 bhavin192 force-pushed the bhavin192:add-contributing branch from 9b85660 to 31117a3 Apr 4, 2019

Show resolved Hide resolved CONTRIBUTING.md Outdated
Show resolved Hide resolved CONTRIBUTING.md Outdated
Show resolved Hide resolved CONTRIBUTING.md Outdated
Show resolved Hide resolved CONTRIBUTING.md Outdated
@PrasadG193

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Add section to run BotKube outside cluster
the steps are:

  • Make BotKube binary (we should probably add make rule for that)
  • Modify config.yaml placed at project root directory
  • Export CONFIG_PATH={path where config.yaml is placed}
  • Make sure that correct context is set and you are able to access k8s cluster
  • run ./botkube binary
@bhavin192

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@PrasadG193 Thanks, will consider adding that.

@mugdha-adhav
Copy link
Member

left a comment

  • To avoid build failures in travis CI, run ./hack/verify-*.sh to check if the code is properly formatted, linted & vendor directory is present.

  • And also mention - if the PR is approved, please squash all the commits and rebase with develop for your PR to be merged

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

To avoid build failures in travis CI, run ./hack/verify-*.sh to check if the code is properly formatted, linted & vendor directory is present.

Yes, we should add that, thanks.

if the PR is approved, please squash all the commits and rebase with develop for your PR to be merged

Personally I don't think squashing all the commits every time is a good idea, if you take a look at this PR transtats/transtats#160. It would have been hard to find out which change is done for what reason if we squash all the commits. I like to leave it to the contributor to decide which commits go to the devel branch. If there are small fixing commits we can always ask them to squash those commits into proper commits. Doing this also helps one to find out reason for a change by running git blame filename

@mugdha-adhav

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Personally I don't think squashing all the commits every time is a good idea, if you take a look at this PR transtats/transtats#160. It would have been hard to find out which change is done for what reason if we squash all the commits. I like to leave it to the contributor to decide which commits go to the devel branch. If there are small fixing commits we can always ask them to squash those commits into proper commits. Doing this also helps one to find out reason for a change by running git blame filename

Yes, rather than asking to squash all commits, we can request to squash the unimportant commits.

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Yes, that makes more sense. We can say something similar

@bhavin192 bhavin192 force-pushed the bhavin192:add-contributing branch 3 times, most recently from a208776 to 606956d May 9, 2019

@bhavin192 bhavin192 requested review from mugdha-adhav and ssudake21 May 9, 2019

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@PrasadG193 can you please check the instructions of 'Build and run BotKube locally' once, as I don't have Mattermost/Slack workspace configured with BotKube to test that.

Show resolved Hide resolved CONTRIBUTING.md Outdated
Show resolved Hide resolved CONTRIBUTING.md Outdated
Show resolved Hide resolved CONTRIBUTING.md Outdated
Show resolved Hide resolved CONTRIBUTING.md Outdated
Show resolved Hide resolved CONTRIBUTING.md Outdated

@bhavin192 bhavin192 force-pushed the bhavin192:add-contributing branch from 606956d to 97be91a May 10, 2019

@bhavin192 bhavin192 requested a review from ssudake21 May 10, 2019

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

This PR depends on #89

@bhavin192 bhavin192 force-pushed the bhavin192:add-contributing branch from 97be91a to 40863bb May 13, 2019

@ssudake21

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@bhavin192 Can you please rebase your branch with develop?

@bhavin192 bhavin192 force-pushed the bhavin192:add-contributing branch from 40863bb to 5be99a5 May 15, 2019

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@ssudake21 done

@bhavin192 bhavin192 force-pushed the bhavin192:add-contributing branch from 5be99a5 to 48841a5 May 17, 2019

@ssudake21

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Thanks. LGTM

@PrasadG193

This comment has been minimized.

Copy link
Member

commented May 17, 2019

@bhavin192 can you please rebase with develop?

Add CONTRIBUTING.md
- This makes it easy for someone new to start working on BotKube
- Adds instructions to build BotKube from source code
- Based on
  https://github.com/jaegertracing/jaeger-client-python/blob/master/CONTRIBUTING.md
  which is Apache 2.0 licensed

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>

@bhavin192 bhavin192 force-pushed the bhavin192:add-contributing branch from 48841a5 to 4c98a39 May 17, 2019

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@PrasadG193 yes, done :)

@PrasadG193 PrasadG193 merged commit b284488 into infracloudio:develop May 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.