add fetchRevisionshook to deploy and activate pipelines - #323

I don't necessarily disagree with the position you've placed this hook, but what is the rationale for where in the pipeline it should be added?

I based it off #209

I'm +1 on this. I'd like the other team members to weigh in.

Curiosity, what is the use case?

I wonder if it would be better to call fetchRevision after activate hooks so you get the most recent data?

@ghedamat the activate hook will supply the activated revision key so you will still have everything you need. Also you may want to check for the revision before you try to activate it.

Also in my case I want to get the previously activated revision so I can generate a change-log. Once activate runs I wont have that info anymore.

@sethpollack I figured that was the case

@lukemelia I think this is an example where order of hooks might be debatable (by other plugin authors)

but I'm +1 to merge

I am 👍 on this. Most likely it's enough to have fetchRevisions run directly before activation as the rest of the hooks shouldn't affect the revisions at all.

We also supported previousActivatedRevision in Redis, azure-tables - what's the use case besides generating the diff?I don't mind the change, but I would want to keep an eye on the actual impact

On Fri, Dec 4, 2015 at 7:49 PM, Michael Klein [email protected]
wrote:

I am 👍 on this.

Reply to this email directly or view it on GitHub:
#323 (comment)

The reason behind that was that you most often need information about the deployed revisions when doing an activation (e.g. for displaying changes or checking if if the revision to activate is present at all). I already did a pr for this a while ago because I thought merging the revision information onto the deployment context was a good idea to check for available revisions in the s3-index plugin. We decided against that as we thought that a private function in the plugin works as well.

As it turns out now it's also useful for regular users to access "deployed revisions" information (for creating changesets, creating a message if an activation is a new deployment or a rollback etc.). When thinking about it the state of 'available revisions' definitely is also part of the whole 'deployment context'. I am looking forward to this change 👍

I think we should have as much information as possible available on the deployment context just to make it easy for users to customize. Also finding the previous activated revisions (in a private plugin method) most likely involves fetching the deployed revisions so I don't think it hurts to put this information on the deployment context as well (as we are requesting the information internally anyway).

I think we should have as little information as possible available on the deployment context - we should do our best to support the use cases in a generic way, but not give any additional information - every piece of information should be seen as a burden you have to support.

I'm 👍 on the approach and implementation, but I'm a little skeptical if we don't see a use case that we otherwise couldn't cover, or a group of existing use cases which are easier/more intuitive to tackle generically.

With that, I'll leave it to you ladies & gents to make a decision on this.

FYI, there has been some more back and forth about among core contributors off-Github and we should have a decision and plan very soon.

Ok awesome, thanks!

@sethpollack et al. We have decided to update pipelines to add a new hook fetchInitialRevisions and to include fetchRevisions in some pipelines where it isn't now.

The new fetchInitialRevisions hook will be added at the following locations:

• deploy pipeline: inserted before willUpload
• activate pipeline: inserted before willActivate

The fetchRevisions hook will be added at the following locations:

• deploy pipeline: before didActivate if running with --activate or before didDeploy if running without --activate
• activate pipeline: before didActivate

fetchInitialRevisions would populate revisionData.initial on the context, and fetchRevisions would populate revisionData as plugins do today. This should provide enough richness for plugins to implement changelogs, clear notifications, etc.

@sethpollack Are you interested in implementing these changes?

@lukemelia Yeah, I can implement it.

Ideally there'd be some legacy check where ECD checks whether plugins
implement fetchRevisions but not fetchInitialRevisions - it would then wrap
the secondary promise to fill revisionData.initial as detailed above, with
a warning
The alternative scenario is working with all plugin authors to move every
plugin that implements the fetchRevisions hook and update it - not
unfeasible, there'll be about 10 at maximum I assume

On Tue, 15 Dec 2015 16:10 Seth Pollack [email protected] wrote:

@lukemelia https://github.com/lukemelia Yeah, I can implement it.

—
Reply to this email directly or view it on GitHub
#323 (comment)
.

@duizendnegen The only think that would suffer if fetchInitialRevisions is plugins which rely on it (of which there are zero so far), so I think there will be some natural time delay and natural peer pressure that will help address this via your "alternative" scenario.

@sethpollack Thanks for your work on this and your patience. Can you squash this into one commit, please?

@lukemelia done!

Thanks for your work and on this @sethpollack!

Thanks!