Add: activate pipeline runs fetchRevisions-hook - #209

Generally makes sense but I'm curious what the actual use case is here, since you need to provide a revision key to the activate pipeline anyway.

Most plugins that implement the activate hook would check if the revision you want to activate is already uploaded. You can get these revisions by running the fetchRevisions-hook or by running this functionality inside the activate-hooks.

I think in most scenarios it makes more sense to build on the already implemented fetchRevisions-hook of the plugin because that will merge the revision data into the deployment context and it's easy to check the deployment context in following hooks.

fetchRevisions would then also need to run on the deploy command because you'd need the revisions data in the check that happens in activate when passing --activate=true to the pipeline. I will update this PR accordingly

Hmm, I would think the activate hook would optimistically assume it had
access to a valid revision and handle the error if that turns out to not be
the case.

On Sunday, August 30, 2015, Michael Klein [email protected] wrote:

Most plugins that implement the activate hook would check if the revision
you want to activate is already uploaded. You can get these revisions by
running the fetchRevisions-hook or by running this functionality inside
the activate-hooks.

I think in most scenarios it makes more sense to build on the already
implemented fetchRevisions-hook of the plugin because that will merge the
revision data into the deployment context and it's easy to check the
deployment context in following hooks.

fetchRevisions would then also need to run on the deploy command because
you'd need the revisions data in the check that happens in activate when
passing --activate=true to the pipeline. I will update this PR accordingly

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

Not sure how the pipeline should then know of the error or how to handle it.

In the redis case 'current' would simply point to an invalid value and in the S3 case you'd try to copy the content of a non existent file to index.html in the index bucket.

The only way you'd know that the passed revision to activate was invalid is to get a list of valid revisions and compare that to the passed revision. that's what the fetchRevisions-hook is passing into the deployment context (existing revisons).

You need a list of valid revisions in the activate hook. When the user passes an invalid revision you need to stop the pipeline and error out. If we run the fetchRevisions hook we will have the available revisions available via the deployment context. If not we would need to implement a method that lists the revisions internally and use that. This is most likely what fetchRevisions uses internally and its much easier and more dry to use the deployment contest written by fetchRevisions instead of calling the internal method in the activate hook imo.

Does this mean that in order for the activate hook to work, the user needs to have installed a plugin that implements the fetchRevisions hook?

Which means that the plugin developer needs to have understood this conversation we are having right here?

Is that going to pose a bit of a disconnect?

No. Activate will just run the fetchRevisions hook so that an index plugin that needs a list of revisions when running activate can get these revisions via the deployment context.

If a plugin developer decided to use a private method to get a list of available revisions or does not care about valid revisions the developer can just ignore the fetchRevisions hook and not implement it.

@LevelbossMike I had been thinking that in the redis case, the redis plugin can validate the specified revisionKey by attempting to read from the key and making sure there is something there. In the s3-index case, if the call to copyObject fails due to the source file not existing, then the revision key is not valid.

I feel comfortable with either approach. I don't think including the fetchRevisions hook is strictly necessary, and think it's strange to depend on it in the ember deploy [target] --activate case. So I think it's unnecessary, but not incorrect.

mmh. makes sense. I'll change the s3-index implementation accordingly.