Regaez / grav-plugin-api

A REST API plugin for GravCMS
MIT License
27 stars 6 forks source link

chore: investigate if API triggers git sync #80

Closed Regaez closed 4 years ago

Regaez commented 4 years ago

For users who are controlling their site content with git sync, it would be great if changes made via the API would also trigger the sync actions.

We should investigate how the git sync is triggered (I'm assuming through Events) and see if the API already triggers these. If not, we should add perhaps fire some custom events based on the action performed.

djairhogeuens commented 4 years ago

Hi @Regaez, I investigated this last week as we are also using the GitSync plugin. The answer is no, git sync is not triggered. GitSync listens to admin page save events and also a custom 'gitsync' event. Modifying pages through the API does not trigger any of these events. There also does not exist a Grav Core event that is triggered everytime a page is saved (that would have been great, see https://github.com/getgrav/grav/issues/2907).

What solution do you propose?

I see a number of options:

We need a solution for this matter so I can make time to implement a solution :)

Regaez commented 4 years ago

@djairhogeuens thanks a lot for looking into this! I really appreciate it. :tada:

Regarding your suggestions:

  1. I don't like the idea of having additional endpoints purely for git sync purposes. It feels... wrong. We'd be externalising a process which should be seamless/happen behind the scenes. The client using the API shouldn't be responsible for keeping the site's content in sync with version control.

  2. I'm not sure that we should be emitting other plugins' events. Certainly none of the core Grav events make sense to emit. I think it's disingenuous for the API plugin (for example) to emit an onAdminSave event -- since there wasn't actually the same scenario/conditions of an admin save (and we might accidentally trigger unwanted actions listening for those events).

  3. I think this is the most suitable approach. We already have an issue for creating custom events that were triggered by the API, so that third party plugins could hook onto these (see #39). I hadn't been prioritising that issue since I had no idea what plugins would exist and use them (but now we have a valid use case!). Tackling that issue first would make sense, after which we could create another plugin (perhaps grav-plugin-api-sync?) with dependencies on the API and Git Sync plugins means that the core API plugin doesn't have a dependency on Git Sync (I think this is important as not everyone uses the sync plugin and it's not essential to the core API functionality). As you mentioned, the connector plugin would simply hook into the respective API events and trigger the appropriate Git Sync method (maybe synchronise()?). Hopefully it's as straightforward as it sounds!

  4. I don't think this is ideal for the same reasons as the second point.

What do you think?

djairhogeuens commented 4 years ago

@Regaez yeah I was just putting all the options I see out there without a clear preference to get your honest opinion ;)

The first was a solution proposed by someone here at the company but I also dislike it because of the exact samen reason as you: introduction of coupling between client and GitSync and bad separation of concerns.

I saw something in the second option because it would not require the introduction of an extra integrator plugin (because indeed, you want to keep the API plugin and the GitSync plugin decoupled). Firing the onAdminSave event is indeed a bad idea though because of the reasons you mentioned. However, the GitSync plugin dies listen to a 'gitsync' event which results in triggering the sync operation. Imho this is a little bit wrong use of events because an event should be the result of an operation while this gitsync-event is actually a command. Firing it (through configuration, not hardcoded) should therefore be safe as, normally, GitSync would be the only subscriber and no other unforeseen side-effects should occur. However, the genericity of such as solution (configuring events to fire per endpoint) is questionable because it actually assumes the wrong usage of events (using them as commands instead of events) and will therefore probably not be usable for other plugin integration.

The third option was my original approach as well. That's why I was first trying to find a Grav Core event that fires on every page but unfortunately that does not exist (yet). The solution of firing custom API events and introducing a third integrator plugin therefore indeed seems like the best option to me as well (until a Grav Core event would be available that could be used instead of the custom API event).

I'll take a look at the other issue regarding API events as well and try to have an implementation ready somewhere this week.

Regaez commented 4 years ago

Hmm. Considering the Git Sync plugin advertises the feature in it's readme: "Any 3rd party plugin can integrate with Git Sync and trigger the synchronization through the gitsync event." I think we would probably be fine to fire this event in order to trigger the sync.

We could, therefore, simply have a configuration option under each endpoint that, if set, would fire the event... But to be honest, I still think that having an integrator plugin hooking into events is a better way to go since this functionality is not strictly necessary for the "core" API feature set and could easily be added through the installation of another small plugin (just like Grav tries to keep itself "leaner"), if so desired.

We can add some documentation (I'm thinking we should add a docs/FAQ.md file) that will guide users to the new plugin if they desire that feature. I'll create a new issue for this.

So yeah, that said, I still think approach 3 is the way to go. I look forward to reviewing your contributions! 👍

djairhogeuens commented 4 years ago

@Regaez as discussed I created a new integrator plugin that hooks into the create, update and delete events from the Api plugin and fires the gitsync event as a consequence. You can find the plugin repository here: https://github.com/djairhogeuens/grav-plugin-api-git-sync-integrator

I tested it and everything is working nicely :) So think we can close this issue (apart from maybe adding a reference to the integrator plugin here). The new plugin already references both the Api plugin and the Git Sync plugin.

Regaez commented 4 years ago

Brilliant! I had a look at the integrator plugin -- it's a nice and tidy little plugin. I've already added a note to the FAQ issue to advertise it. I'll also keep in mind to send some PRs in the future if there are any endpoints/events added that would require syncing.

Thanks again for investigating this issue and your contributions to the project!