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

[18.01] Webhooks improvements #5342

Merged
merged 10 commits into from Feb 1, 2018

Conversation

Projects
None yet
4 participants
@dannon
Member

dannon commented Jan 19, 2018

This deconflicts and catches #5164 up to be mergeable with dev. I checked with @anatskiy who OK'd the rebase.

See #5164 for more details, also copied below.


This PR improves and simplifies the overall Webhooks' structure.

  1. All the webhook's files now are located in one folder (no more config, helper and static subfolders).
  2. Function Webhooks.load() is unified even more for all endpoints (e.g., onload, masthead, tool, etc.).
  3. Webhooks have a weight now (like it was explained here).
  4. Some of the logic has been moved to the client (like it was suggested here).

If you agree to the changes, I'll update the documentation accordingly.

P.S. Every single webhook is broken now. I've updated all the demo webhooks in this PR, but others won't work.

@dannon

This comment has been minimized.

Member

dannon commented Jan 19, 2018

Fixing linting issues now that the tests are running. Also looks like potentially legit qunit failures; will check them out in the morning

@dannon

This comment has been minimized.

Member

dannon commented Jan 19, 2018

After the last few commits, QUnit tests pass for me locally now. Letting test suites run and then I'll revisit the TDT tests.

(edit the last: Good to go now)

@martenson

This comment has been minimized.

Member

martenson commented Jan 25, 2018

P.S. Every single webhook is broken now.

@anatskiy I am willing to merge this so it makes it to 18.01 but I would like to see documentation for the new and also for the transition from the old, so people can update their webhooks more easily.

With any feature that is a bit more mature I would argument for at least one deprecation release -
webhooks are on the borderline for me.

@martenson

This comment has been minimized.

Member

martenson commented Jan 25, 2018

We have branched the 18.01, if this wants to make it there, it needs to re-target to that branch (and be merged soon).

@dannon dannon changed the base branch from dev to release_18.01 Jan 25, 2018

@dannon

This comment has been minimized.

Member

dannon commented Jan 25, 2018

@martenson I had it targeted and milestoned; per the rules it probably should have been bumped or merged prior to the branch. Not a big deal though, retargeted.

@martenson

This comment has been minimized.

Member

martenson commented Jan 25, 2018

I think we delayed Evgeny's PR enough so I am willing to break the rule here. It would be cool to have webhooks mature for the GCC18 and this PR goes in this direction but if the branching rules are above both of these for you, please raise your voice @dannon

@dannon

This comment has been minimized.

Member

dannon commented Jan 25, 2018

@martenson I think you misinterpreted my words, but no worries. As I said above, I retargeted this (to the 18.01 branch) and would definitely like for someone to merge it. Preferably not me, since half of these commits are mine at this point.

@anatskiy

This comment has been minimized.

Contributor

anatskiy commented Jan 25, 2018

@dannon thanks a lot for the revision! Everything webhooks-related seems to work well. The search plugin is broken, though. I'll fix it soon.

@martenson hm, what do you mean by mature here?

I'll update the documentation regarding the simplified plugin structure.

@dannon

This comment has been minimized.

Member

dannon commented Jan 25, 2018

@anatskiy Sure, anytime!

@martenson

This comment has been minimized.

Member

martenson commented Jan 25, 2018

@anatskiy I mostly mean 'stable'/'not getting breaking changes in future'.

@anatskiy

This comment has been minimized.

Contributor

anatskiy commented Jan 25, 2018

@martenson oh, I see. Right, it won't change that much anymore.

@nsoranzo nsoranzo changed the title from Webhooks improvements to [18.01] Webhooks improvements Jan 25, 2018

@anatskiy

This comment has been minimized.

Contributor

anatskiy commented Jan 28, 2018

@dannon I pushed a fix dannon#25

@martenson martenson merged commit ffaabe0 into galaxyproject:release_18.01 Feb 1, 2018

5 of 6 checks passed

api test Build finished. 342 tests run, 4 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 171 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 79 tests run, 4 skipped, 0 failed.
Details
selenium test Build finished. 118 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@martenson

This comment has been minimized.

Member

martenson commented Feb 1, 2018

Thanks @anatskiy ! Could you please write a paragraph or so of docs for folks that have custom webhooks in old format what they need to go through? We will include it in the release notes. Thanks.

@anatskiy

This comment has been minimized.

Contributor

anatskiy commented Feb 1, 2018

@martenson oh, I totally forgot about it. I'll do it!

@martenson

This comment has been minimized.

Member

martenson commented Feb 1, 2018

@anatskiy awesome, once you have it please xref it to #4920

@anatskiy

This comment has been minimized.

Contributor

anatskiy commented Feb 12, 2018

@martenson here are a few words on the migration to the new Webhooks format.


To make old Webhooks work with the recent changes, you need to do the following:

  • rename the main config file: <name>.yml -> config.yml
  • in the config file, rename the name attribute: name -> id
  • put all files into the plugin's root folder, which should contain only four files: config.yml (mandatory), __init__.py (optional), script.js (optional), styles.css (optional)

xref #4920

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment