Improved plugin run control - #349

@ef4 Thanks. I generally like the proposed API. Am I reading this PR correctly to be an addition to the plugins list? Is your suggestion that the plugins list be deprecated?

I'd like to review some real world deploy config files and see how they would change based on this.

Also, if we decide that we want to merge this for the approaching 0.6.0 beta, we'll need a docs PR to go along with this, which can be PR'd against #335.

@achambers @duizendnegen @ghedamat thoughts?

Yes, I think we should also deprecate the plugins option, once we have a solid story here, including docs.

Hm, the other function provided by the plugins list is the ability to include a plugin more than once by aliasing it. This new API doesn't provide a mechanism for that.

not used the plugins personally, so can't add on what features are not covered by this work-flow;
we discussed during the F2F a way to suppress pipeline steps for certain plugins (i.e. disable fetchRevisions for azure-tables) - that's a possible add-on

thanks for this @ef4, it looks great!

a few comments

1. I think we should try to release a 0.6.0-beta.1 before this is merged and maybe as soon as display progress notification during deploy #280 is ready (reason being that I believe some people or at least myself are using current master in production and this will be a breaking change). Alternatively I can just lock my apps to a specific commit if you guys think this is not needed.

2. I know for fact that there are users using the plugins aliasing feature so we should have a story for that.

3. I love the disabled option as part of ember-cli-deploy proper instead of being in the plugins, we should identify what plugins have implemented a disabled option in the meanwhile and remove it there

currently I know that this happens in

• ember-cli-deploy-slack

and possibly other plugins (feel free to edit the list)

4. I'm not too concerned about reserving the disabled, runBefore and runAfter top-level keys, AFAIK none of our core plugin uses any of them.

5. happy to beta-test this in the ember-deploy-demo app and possibly on some clients apps too

Awesome work @ef4. Thanks. I'm happy with the proposed API once we include the ability to alias plugins.

One question that comes to mind is, if I have quite a few plugins listed, could it become confusing to me to see the order in which things will run if I have runBefore and runAfter defined in each plugin's config block?

One of the suggestions in #307 was that the plugins property could take a function which is passed the array of plugins and the user can reorder, alias etc etc all there in the one place.

Not saying that that way is better, I like your approach. Just wondering if this is something we need to consider.

FYI, we're going to target this for 0.7 since we've got some things to work out with it still and 0.6.x is ready for beta.

@ef4 thanks again for this!

do you have any spare cycles and/or need input from us to take it to completion?

otherwise we can try to take it over from here if that's easier, you did all the hard work already :D

I haven't had spare cycles lately, feel free to jump in if you are able to help sooner. The piece that still needs to be done is a new aliasing API. I think it should look like:

ENV.alias = [
  // This will cause the build module to run twice, looking
  // up its config under the two given names 
  { module: 'build', as: ['desktop-build' , 'mobile-build']},

  // If you want the original name to still be available too, 
  // it should be included in the list. This will run s3 twice, 
  // once using the config under "s3" and once under "special-s3"
  { module: 's3', as: ['s3', 'special-s3' ] },

  // a singleton should also be accepted and automatically 
  // expanded to a list of length one
  { module: 'foo', as: 'bar' }
];

sounds good! I'll see if some of us can take it to completion and we'll then ping you for review!

I'll adopt this adorable child.

Ok, some great discussion here. After chatting with @ghedamat today, I'm going to assume custody of this little rascal from @lukemelia so we can try and get this over the line for Ember Camp.

The one thing that wasn't addressed, aliasing of plugins, seems to have a reasonable solution proposed above by @ef4.

I'm happy with all of the above however I have one or two thoughts:

1. In terms of aliasing, the term module is not a term that has been used in conjunction with ember-cli-deploy yet at all and might throw people. I propose using plugin instead.
2. Seeing as the proposal for aliasing is to add a new property to the top level ENV object, it makes me wonder whether it makes sense to then move the ordering and disabling of plugins to a top level property too for consistency. I guess the two examples would look something like the following:

Inline before, after and disabled

module.exports = function(deployTarget) {
  var ENV = {
    alias: [
      { plugin: 's3', as: ['s3', 's3-backup'] }
    ],

    build: {
      environment: 'development',
    },

    'revision-data': {
      type: 'git-commit',
      after: 'build',
    },

    s3: {
      accessKeyId: process.env.AWS_KEY,
      secretAccessKey: process.env.AWS_SECRET,
      bucket: 'my-assets',
      region: 'eu-west-1',
      before: 'redis',
      disabled: deployTarget === 'development',
    },

    's3-backup': {
      accessKeyId: process.env.AWS_KEY,
      secretAccessKey: process.env.AWS_SECRET,
      bucket: 'my-assets-backup',
      region: 'eu-west-1',
      before: 's3',
      disabled: deployTarget === 'development',
    },

     redis: {
       allowOverwrite: true,
     },

     'ssh-tunnel': {
      username: "ec2-user",
      host: "foo-bar.us-west-2.compute.amazonaws.com",
      dstPort: 6379,
    }
  };

  return ENV;
};

Top level before, after and disabled

module.exports = function(environment) {
  var ENV = {
    pipeline: {
      alias: {
        s3: { as: ['s3', 's3-backup'] },
      },

      runOrder: {
        'revision-data': { after: 'build' },
        s3: { before: 'redis' },
        's3-backup': { before: 's3' },
      },

      disabled: {
        s3: deployTarget === 'development',
        's3-backup': deployTarget === 'development',
      },
    },

    build: {
      environment: 'development',
    },

    'revision-data': {
      type: 'git-commit',
    },

    s3: {
      accessKeyId: process.env.AWS_KEY,
      secretAccessKey: process.env.AWS_SECRET,
      bucket: 'my-assets',
      region: 'eu-west-1',
    },

    's3-backup': {
      accessKeyId: process.env.AWS_KEY,
      secretAccessKey: process.env.AWS_SECRET,
      bucket: 'my-assets-backup',
      region: 'eu-west-1',
    },

     redis: {
       allowOverwrite: true,
     },

     'ssh-tunnel': {
      username: "ec2-user",
      host: "foo-bar.us-west-2.compute.amazonaws.com",
      dstPort: 6379,
    }
  };

  return ENV;
};

I think the examples above are an semi decent reflection of something that may happen in prod. Any suggestions to improve it are welcome.

Looking at the examples, I'm actually favouring the top level props for the run order and disabled. It makes it much easier to reason the order that the plugins might run in rather than looking for the particular property across all the plugin configs. Also, find out which plugins are disabled and when is all in one spot too. So, my personal preference is the top level props.

@ghedamat / @lukemelia / @ef4 have any final thoughts on this? Are you happy for me to go ahead with the above?

And once again, thanks @ef4 for your cracking contribution. We' re always appreciative :)

For me to remember

• Ensure ordering of aliased plugins works
• Ensure disabling of aliased plugins works
• Ensure we're happy with the api for ordering for both author defined and user defined config
• Show warning if no installed plugins
• Show warning if uninstalled addons referenced by alias
• Show warning if uninstalled addons referenced by runOrder
• Show warning if uninstalled addons referenced by disabled
• Deprecate plugins property - this should override the above if it exists.
• Rebase with required hook validation and fix conflicts
• Remove this from ember-cli-deploy-s3 and reimplement with runBefore
• Decide on what happens when a user aliases a plugin that has run{Before,After} by plugin author. Also, if runBefore is set and so is pipeline.runOrder, work out which should win (probably config)

@lukemelia Aaron is pretty much done here and I already tested in my demo app

if you can give it for a quick spin I'd say we're good to merge and cut the first beta :D

Worked like a charm. New config is far more pleasant for my use case. Thanks @achambers!

🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆