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

Improve readme #398

Merged
merged 14 commits into from
Mar 23, 2016
Merged

Improve readme #398

merged 14 commits into from
Mar 23, 2016

Conversation

begriffs
Copy link
Contributor

This PR addresses some things left out of the initial readme.

  • Change the doc urls to their soon-to-be home: https://www.citusdata.com/docs/citus/current
  • Chrome did not understand our irc:// link so I changed it to a web client for freenode
  • Add a note about how not all SQL commands are supported (so nobody feels let down)
  • Added a CONTRIBUTING.md with build instructions
  • A simple copyright footer for the readme
    • I made up the numbers 2012-2016, are they correct?
  • Add a list of who's using Citus
    • I chose to omit logos to make this section more compact. Could make it even more compact by using a table rather than an HTML definition list
  • Did not add the section about CloudFormation
    • It's a good looking section but our tutorials need changes to be compatible with it
    • Don't want people to be confused about what to do next after starting CloudFormation
  • The commits are logically separated but I can squash them into one if desired

/cc @ozgune


```bash
./configure
make
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set the PostgreSQL path here? -- don't know, just asking

Copy link
Contributor

Choose a reason for hiding this comment

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

path was not necessary since brew install postgres creates pg_config in path. However I had to run:
brew link openssl --force to create links to openssl headers.

@jasonmp85
Copy link
Contributor

Change the doc urls to their soon-to-be home: https://www.citusdata.com/docs/citus/current

Shouldn't they point at 5.0? This README will live in the 5.0 packages. Maybe we make it point to 5.0, then change it back to current in the master branch?

3. Build and test

```bash
./configure
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to first run:cd citus before running the rest of the steps.


```bash
# on mac
brew install postgresql
Copy link
Contributor

Choose a reason for hiding this comment

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

This installs postgresql-9.4.4, not postgres-9.5. It's OK as-is, but might be useful to see if we can brew install postgres 9.5

Copy link
Member

Choose a reason for hiding this comment

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

Oops @sumedhpathak , regression tests fail on 9.4.5 (i.e., < 9.4.6). see #360 for the details.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see the same errors @onderkalaci mentioned. So @begriffs we should figure out how to get postgres-9.5 via brew as opposed to postgres-9.4.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Uhhhhh? I'm not sure about any of this.

@sumedhpathak — when is the last time you ran brew update? Homebrew doesn't have multiple versions of packages, just the latest. And PostgreSQL is on 9.5.1.

@begriffs — What is postgresql-9.4? Homebrew just has a postgresql package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating brew instructions to include an update and installation of the postgresql package.

```bash
./configure
make
make install
Copy link
Contributor

Choose a reason for hiding this comment

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

Two notes here:

  • make install needs to be run as sudo
  • make check gives me an error both here, and with my manual install. I've never run make check and I don't know what its supposed to do. Should we direct them to src/test/regress and run the regression tests there? Alternately let's remove this from here, and put a link after this to our documentation for further setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, sudo is only required for Linux…


___

Copyright 2012-2016 Citus Data
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this copyright information for the README or for the entire codebase?

If the former, just put Copyright 2016.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entire codebase; so I should leave it as-is?

@begriffs
Copy link
Contributor Author

Merging this by @ozgune's request.

@begriffs begriffs merged commit a7d3b79 into release-5.0 Mar 23, 2016
@jasonmp85 jasonmp85 deleted the readme-polish branch March 23, 2016 23:29
DimCitus pushed a commit that referenced this pull request Jan 10, 2018
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.

5 participants