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 postgres datastore type #5403

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

JamesChristie
Copy link

@JamesChristie JamesChristie commented Aug 27, 2018

This is work that aims to integrate changed submitted here and here. No changes have been made to the gx package hashes as the work it depends upon has not yet been accepted.

@JamesChristie JamesChristie force-pushed the postgres branch 2 times, most recently from df6caaa to ce3bfc2 Compare August 27, 2018 18:36
@Stebalien Stebalien added the kind/feature A new feature label Aug 27, 2018
@Stebalien
Copy link
Member

Thank you for the PR! Just a bit of a heads-up, we're a bit oversubscribed on quite a few fronts at the moment and the changes to sql-datastore are non-trivial so it may be a while before we get a chance to do a thorough review.

Quick things:

  1. You can run gx release patch in the dependencies yourself to release new (non-canonical) versions yourself.
  2. You can then update these dependencies here by running gx update $NewHash. Getting the tests to pass first will help speed up the review process significantly.

@JamesChristie
Copy link
Author

I'm planning to do just that as soon as the PRs this depends on are merged! As soon as those are done, I'll update this pull; for the time being, it's here for posterity's sake.

}

type postgresDatastoreConfig struct {
path string
Copy link
Member

Choose a reason for hiding this comment

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

path isn't needed here either.

}

func (postgresDatastoreConfig) Create(path string) (repo.Datastore, error) {
pg := postgresdb.Options{}
Copy link
Member

Choose a reason for hiding this comment

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

This should get Host/User/Password/Database from config.

When initializing node with this backend user could pass those using env vars. in config profile we'd just call os.Getenv.

We then could provide a simple script like the one below to avoid storing passwords in shell history

#!/bin/sh

[check args]

export IPFS_PG_HOST=$1
export IPFS_PG_USER=$2
export IPFS_PG_DATABASE=$3
printf "${IPFS_PG_USER}@${IPFS_PG_HOST} password:"
stty -echo
read IPFS_PG_PASSWORD
stty echo
export IPFS_PG_PASSWORD

ipfs init --profile=postgres

Choose a reason for hiding this comment

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

Thanks for the feedback! Making these changes now. I think we could also use the PGPASSFILE env variable to avoid directly storing the password as an env variable.

func (c *postgresDatastoreConfig) DiskSpec() DiskSpec {
return map[string]interface{}{
"type": "postgresds",
"path": c.path,
Copy link
Member

Choose a reason for hiding this comment

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

path is not needed here, but I'm not sure what to put here that makes sense. My guess would be user+database, as host/password may change frequently in some setups.

Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long and you having to fight with gx, but this is getting there.

Some things in other places:

There are also some merge conflicts here, if you don't want to fight with them, once other review comments are addressed I can fix them for you (it will likely involve gx too)

But other than that it's looking good.

.gx/lastpubver Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
plugin/plugins/postgresds/postgresds.go Outdated Show resolved Hide resolved
plugin/plugins/postgresds/postgresds.go Outdated Show resolved Hide resolved
plugin/plugins/postgresds/postgresds.go Outdated Show resolved Hide resolved
plugin/plugins/postgresds/postgresds.go Outdated Show resolved Hide resolved
plugin/plugins/postgresds/postgresds.go Outdated Show resolved Hide resolved
scripts/ipfs_postgres.sh Outdated Show resolved Hide resolved
@i-norden
Copy link

i-norden commented Jan 8, 2019

I believe I've addressed all of the above issues. The required sql-datastore PR has been merged but the go-ipfs-config PR still needs to be merged, iptb-plugins will then need to be gx-ified with the new go-ipfs-config hash, and then finally go-ipfs can be gx-ified with the new go-ipfs-config and iptb-plugins hashes.

Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

(1 nit + needs rebase), rest looks good

@@ -8,3 +8,4 @@ ipldgit github.com/ipfs/go-ipfs/plugin/plugins/git 0
badgerds github.com/ipfs/go-ipfs/plugin/plugins/badgerds 0
flatfs github.com/ipfs/go-ipfs/plugin/plugins/flatfs 0
levelds github.com/ipfs/go-ipfs/plugin/plugins/levelds 0
postgresds github.com/ipfs/go-ipfs/plugin/plugins/postgresds 0
Copy link
Member

Choose a reason for hiding this comment

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

While it may be useful for quite a few people, I wouldn't preload this plugin by default (leaving commented out is fine)

Choose a reason for hiding this comment

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

Thank you for all the help, magik6k!

@magik6k
Copy link
Member

magik6k commented Jan 12, 2019

Jenkins seems to be unable to fetch dependencies from you - https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fgo-ipfs/detail/PR-5403/44/pipeline. (gitcop warning needs fixing tee)

@i-norden
Copy link

Jenkins seems to be unable to fetch dependencies from you - https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fgo-ipfs/detail/PR-5403/44/pipeline. (gitcop warning needs fixing tee)

After making some very minor last minute changes to sql-datastore.. I forgot to re-gx-ify the package and so it was merged with the wrong hash :(

My apologies, sql-datastore will need a gx release patch performed.

@i-norden i-norden force-pushed the postgres branch 2 times, most recently from ba74810 to 1534b75 Compare January 24, 2019 18:31
@i-norden
Copy link

sql-datastore has been updated with the correct hash and make gx-deps is passing where it previous failed, but we still run into an issue with the sql-datastore dependency when running the shell script https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fgo-ipfs/detail/PR-5403/51/pipeline.

I am also confused because the travis-ci builds are passing on OSX and only run into this issue with sql-datastore with the Linux builds.

Before this can be merged go-ipfs-config PR needs to be merged and its hash updated here and in iptb-plugins, and then the iptb-plugins hash needs to be updated here as well.

@Stebalien
Copy link
Member

Lol. It looks like gx is case sensitive when it comes to language names.

@Stebalien
Copy link
Member

Try again with: QmZmPKg1RKY2Cy5czKEBJTfDYzenbGww2pFNQKxLSKrPRB

(you're not the first to find this, apparently: whyrusleeping/gx#187)

@i-norden i-norden force-pushed the postgres branch 2 times, most recently from 29e3193 to b09225e Compare January 24, 2019 21:29
@i-norden
Copy link

i-norden commented Jan 24, 2019

Yay that seems to have fixed the problem, thanks! It looks like the QmZmPKg1RKY2Cy5czKEBJTfDYzenbGww2pFNQKxLSKrPRB hash didn't capture the changes from ipfs/go-ds-sql#6 so we might want to gx release patch one more time although it appears the error we are getting now is just due to waiting on the updated go-ipfs-config dependencies.

License: MIT
Signed-off-by: User Name <email@address>
…ies; undo gx releases; address feedback; gx hashes will need to be updated once go-ipfs-config PR is merged and gx released"
@Stebalien
Copy link
Member

That was just a bug that has been fixed in master. I've rebased for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A new feature status/in-progress In progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants