Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Add postgres profile #5

Merged
merged 0 commits into from
Apr 15, 2021
Merged

Add postgres profile #5

merged 0 commits into from
Apr 15, 2021

Conversation

JamesChristie
Copy link

@JamesChristie JamesChristie commented Aug 27, 2018

This is the addition of a profile to utilize the changes being made to this package.

profile.go Outdated Show resolved Hide resolved
@i-norden
Copy link

Sorry for the git mess, tried to clean it up a bit but can't seem to get rid of ab850a5 on this fork. I did not do a gx release this time since I moved away from head state to try and clean up the git history. Is it okay if we do that after we merge? I will then update that gx hash in the go-ipfs PR (and iptb-plugins).

@i-norden
Copy link

Rebasing fixed the git mess, sorry about that!

@i-norden
Copy link

I think this is ready to merge. Would like to get this in so we can finish the go-ipfs postgres merge. Thanks!

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, but other than that LGTM

profile.go Outdated Show resolved Hide resolved
@i-norden
Copy link

i-norden commented Feb 6, 2019

Lmk if there is anything I can do to help get this merged in, thanks!

@magik6k
Copy link
Member

magik6k commented Feb 6, 2019

@Stebalien ping

@Stebalien
Copy link
Member

  1. Do we want to include postgres support by-default or should we make it a plugin? If we go the plugin route, the plugin could register it's own profile.
  2. Using environment variables like this sketches me out a bit. Given all those variables, does this buy anything over manually updating the config? Is this an issue because the datastore_spec file makes changing datastores after initialization difficult?

@magik6k
Copy link
Member

magik6k commented Feb 19, 2019

  1. the plugin could register it's own profile.

👍, but I'm not sure how (preloaded) plugins work with init.

  1. [...] Is this an issue because the datastore_spec file makes changing datastores after initialization difficult?

We write some things to the datastore on init, so we need have a way to access it at init

@i-norden
Copy link

i-norden commented Feb 21, 2019

When this PR was first opened up the Postgres datastore was being built directly into the fsrepo/datastores.go, but then most of the datastores were moved to being plugins and so we followed suite here. As @magik6k points out, if we don't have the Postgres datastore configured with the necessary variables at init we run into an error at that time as it needs to connect and write to the database.

@Stebalien
Copy link
Member

So, we can do this with preloaded plugins. We load them before calling init so they can add profiles in Plugin.Init. Unfortunately, this won't work for non-preloaded plugins. For that, we needhttps://github.com/ipfs/kubo/pull/4244.

@i-norden
Copy link

i-norden commented Jun 18, 2019

Hi all, wanted to check in and see if I could finish up this set of PRs. Not entirely clear to me what else needs to be done. I am personally partial to including the Postgres datastore as one of the preloaded plugins, this makes it straight forward to use Postgres when working with an ipfs node as part of another codebase.

E.g. just need to do:

        l, err := loader.NewPluginLoader("")
	if err != nil {
		return nil, err
	}
	err = l.Initialize()
	if err != nil {
		return nil, err
	}
	err = l.Inject()
	if err != nil {
		return nil, err
	}
	node, err := ipfs.InitIPFSNode(ipfsPath)
	if err != nil {
		return nil, err
	}

And it is able to work with the Postgres backed datastore (if that is what was initialized with ipfs init)

Its not clear to me exactly how we would use it this way otherwise, I assume we would need to build the plugin "manually" and then link to its path in the loader.NewPluginLoader("")?

@Stebalien
Copy link
Member

Postgres is a large dependency to add by default.

Its not clear to me exactly how we would use it this way otherwise, I assume we would need to build the plugin "manually" and then link to its path in the loader.NewPluginLoader("")?

What about ipfs/kubo#6474? You'd run:

        l, err := loader.NewPluginLoader()
	if err != nil {
		return nil, err
	}

	err = l.Load(myPlugin) // magic here.
	if err != nil {
		return nil, err
	}

	err = l.Initialize()
	if err != nil {
		return nil, err
	}
	err = l.Inject()
	if err != nil {
		return nil, err
	}
	node, err := ipfs.InitIPFSNode(ipfsPath)
	if err != nil {
		return nil, err
	}

@i-norden
Copy link

Thanks @Stebalien, that works! With that in mind, is there anything else blocking merging this work?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants