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

support pubgate #111

Merged
merged 6 commits into from Jun 3, 2019
Merged

support pubgate #111

merged 6 commits into from Jun 3, 2019

Conversation

@robjloranger
Copy link
Member

robjloranger commented May 20, 2019

this moves the unmarshaling of a remote actor out into a new helper which
accounts for the possibility of a context being a list or a single entity.
i.e. a string or an object.

basics tests are provided for both situations

also go fmt'd the file activitypub.go

during the course of working on this I found a missing string format variable in exports.go, in fixing that I also go fmt'd the file.

Fixes #100
This also fixes #116 by using the followers Inbox if SharedInbox is not set.

  • I have signed the CLA
@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented May 20, 2019

sorry for the force pushes 😄 I haven't rebased in a while

@robjloranger robjloranger changed the title fix for #100 - can't follow from pubgate fixes #100 - can't follow from pubgate May 20, 2019
@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented May 21, 2019

Oops, I left an extra field in the test table. Let me fix that

this moves the unmarshaling of a remote actor out into a new helper which
accounts for the possibility of a context being a list or a single entity.
i.e. a string or an object.

basics tests are provided for both situations

also go fmt'd the file activitypub.go
@thebaer

This comment has been minimized.

Copy link
Member

thebaer commented May 21, 2019

Thanks! I think the code looks good overall, just need to test. Was this route better than changing the type of Context to an interface{}, do you think?

Also, @mrvdb could you take a look at this?

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented May 21, 2019

I though so at first, after going with this instead I feel they would both be similar.

Either way we need to check for a string or slice and convert any string contexts into a slice. Due to the append behaviour when adding a private key in the core library.

So this could easily be moved there instead if preferred.

@mrvdb

This comment has been minimized.

Copy link
Collaborator

mrvdb commented May 22, 2019

Has this been this tested by someone already or is this a 'theoretical fix' at this time? (perhaps a good time to discuss at some point what is expected in terms of integration tests from contributors?)

@thebaer I will have a look indeed.

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented May 22, 2019

@mrvdb I have not had a chance to manually test the integration.

But I agree this is a good time to start talking about integration tests in general. There would ideally be a suite that runs on pull requests to test automatically, as well as being able to run locally while developing.

@joyeusenoelle

This comment has been minimized.

Copy link
Contributor

joyeusenoelle commented May 22, 2019

TravisCI does run on each commit on a pull request in these repos, to make sure the code's not broken. But that's not necessarily the same thing as "the code does what I want it to". ;)

.vscode/settings.json Outdated Show resolved Hide resolved
this moves the unmarshaling of a remote actor out into a new helper which
accounts for the possibility of a context being a list or a single entity.
i.e. a string or an object.

basics tests are provided for both situations

also go fmt'd the file activitypub.go
@thebaer

This comment has been minimized.

Copy link
Member

thebaer commented May 25, 2019

@robjloranger By the way, the best way to remove files that get accidentally committed like that would be with a new commit, instead of a force-push. Force-pushing is okay for single-user repos, but can mess things up when other people have pulled your branch.

Another helpful hint, if you didn't know about it: using git add -p to stage changes will let you walk through each chunk of code and decide whether or not to include it. This is the way I do all my commits, and has helped a lot over the years.

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented May 25, 2019

@mrvdb could you test by trying to follow @log@rob.writefreely.dev from your pubgate instance? this branch is live there now.

I will try to set one up today and test as well.

@mrvdb

This comment has been minimized.

Copy link
Collaborator

mrvdb commented May 25, 2019

@robjloranger Thanks, I will certainly do so, but I'll only have the ability to do it in about a week or so. (I'm hiking next week)

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented May 25, 2019

ok no trouble, I'll set up a local instance and try it out

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented May 26, 2019

if anyone familiar with pubgate and writefreely could help, I can't figure out how to follow a writefreely user from pubgate.

I assume it is an outbox follow on the pubgate side, but pointed at what endpoint on the writefreely side for a given user I do not know.

@mrvdb

This comment has been minimized.

Copy link
Collaborator

mrvdb commented Jun 3, 2019

Following https://rob.writefreely.dev from pubgate does not error out anymore, so that's a good sign.

The follow action works and the endpoint is correctly entered into the following list on my end. If you can start writing some posts on that endpoint I will monitor if the follow does actually work.

( @robjloranger I find it easiest to use https://www.getpostman.com/ and use the pubgate API that way)

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Jun 3, 2019

@mrvdb OK wrote two test posts. I see both from mast still. And can unfollow and follow from there as well still.

@mrvdb

This comment has been minimized.

Copy link
Collaborator

mrvdb commented Jun 3, 2019

I don't see any connection coming in, can you post again? (I've upped logging so I see more)

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Jun 3, 2019

All set

@mrvdb

This comment has been minimized.

Copy link
Collaborator

mrvdb commented Jun 3, 2019

Nothing on the pubgate side, my follow on mastodon gets the post. I think the error is on my side.

@mrvdb

This comment has been minimized.

Copy link
Collaborator

mrvdb commented Jun 3, 2019

Not sure what is going on. I get objects from my other subscriptions, so pretty sure the setup is ok now.

I've unfollowed and re-followed, could you post again?

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Jun 3, 2019

This issue appears fixed, but we have a new interoperability issue: #116

@robjloranger robjloranger changed the title fixes #100 - can't follow from pubgate support pubgate Jun 3, 2019
robjloranger added 3 commits Jun 3, 2019
@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Jun 3, 2019

all is working well now with pubgate @thebaer

@thebaer thebaer self-requested a review Jun 3, 2019
@thebaer
thebaer approved these changes Jun 3, 2019
Copy link
Member

thebaer left a comment

Awesome, everything looks good to me. Thanks, @robjloranger and @mrvdb! Merging this now.

@thebaer thebaer merged commit c87b7ab into develop Jun 3, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@robjloranger robjloranger deleted the gh100 branch Jun 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.