E528 Improved plugin run control by ef4 · Pull Request #349 · ember-cli-deploy/ember-cli-deploy · GitHub
[go: up one dir, main page]
Skip to content

Improved plugin run control#349

Merged
ghedamat merged 2 commits intomasterfrom
plugin-control
Jul 11, 2016
Merged

Improved plugin run control#349
ghedamat merged 2 commits intomasterfrom
plugin-control

Conversation

@ef4
Copy link
Contributor
@ef4 ef4 commented Jan 20, 2016

This closes #307 with the following replacements for the hard-to-use plugins list:

  • the user can disable individual plugins by setting disabled: true on each plugin's config entry.

    ENV.somePlugin.disabled = true;

  • the user can influence plugin order by expressing inter-plugin dependencies with runBefore and runAfter on each plugin's config entry.

    ENV.somePlugin.runAfter = 'build';
ENV.somePlugin.runBefore = [ 's3', 's3-index' ];

  • plugin authors can influence plugin order by expressing inter-plugin dependencies by setting runBefore and runAfter properties directly on their plugin instance.

    DeployPlugin.extend({
  runAfter: 's3'
});

Backward compatibility issues: this change reserves the properties disabled, runBefore, and runAfter for use in each plugin's configuration entry. If any plugin is already using them, it will result in a conflict. I think this is worth it for the user ergonomics. However, an alternative solution would be to move all of these settings to a separate top-level property, like: ENV.disabled = { somePlugin: true } and ENV.order = { somePlugin: { before: 's3' } }.

@lukemelia
Copy link
Contributor
lukemelia commented Jan 20, 2016

@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?

@ef4
Copy link
Contributor Author
ef4 commented Jan 20, 2016

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

@lukemelia
Copy link
Contributor
lukemelia commented Jan 20, 2016

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.

@duizendnegen
Copy link
Contributor
duizendnegen commented Jan 20, 2016

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

@ghedamat
Copy link
Collaborator
ghedamat commented Jan 20, 2016

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

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

  1. 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.

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

@achambers
Copy link
Member
achambers commented Jan 20, 2016

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.

@lukemelia
Copy link
Contributor
lukemelia commented Jan 22, 2016

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.

@lukemelia lukemelia added this to the 1.0.0 milestone Jan 24, 2016
@achambers achambers mentioned this pull request Feb 9, 2016
Closed
15 tasks
@ghedamat
Copy link
Collaborator
ghedamat commented Mar 1, 2016

@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

@ef4
Copy link
Contributor Author
ef4 commented Mar 1, 2016

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' }
];

@ghedamat
Copy link
Collaborator
ghedamat commented Mar 1, 2016

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

@lukemelia lukemelia self-assigned this Mar 1, 2016
@lukemelia
Copy link
Contributor
lukemelia commented Mar 1, 2016

I'll adopt this adorable child.

@elidupuis elidupuis mentioned this pull request Mar 3, 2016
Closed
@kpfefferle kpfefferle mentioned this pull request Apr 21, 2016
Closed
@achambers
Copy link
Member
achambers commented Jun 1, 2016 •
edited
Loading

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<
10BC0
/span> 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 :)

@achambers
Copy link
Member
achambers commented Jun 4, 2016 •
edited
Loading

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)

@ghedamat ghedamat mentioned this pull request Jun 28, 2016
Merged
3 tasks
@kpfefferle kpfefferle mentioned this pull request Jul 7, 2016
Closed
@ef4 @achambers
Improved plugin run control
dfb6cc7 
This closes #307 with the following replacement for the hard-to-use `plugins` list:

 - the user can disable individual plugins by setting `disabled: true` on each plugin's config entry.
 - the user can influence plugin order by expressing inter-plugin dependencies with `runBefore` and `runAfter` on each plugin's config entry.
 - plugin authors can influence plugin order by expressing inter-plugin dependencies by setting `runBefore` and `runAfter` propreties directly on their plugin instance.
@achambers achambers force-pushed the plugin-control branch 5 times, most recently from 3f6ac06 to c9a42d6 Compare July 7, 2016 23:34
lukemelia
lukemelia reviewed Jul 7, 2016
View reviewed changes
lib/models/plugin-registry.js

/**
* Build up a hash of plugin instance execution order overrides.
* This hash will only container an entry for instances that should run in a particular order. All other instances will be ordered by their natural
Copy link
Contributor
@lukemelia lukemelia Jul 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/container/contain/

@ghedamat
Copy link
Collaborator
ghedamat commented Jul 10, 2016 •
edited
Loading

@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

@lukemelia
Copy link
Contributor
lukemelia commented Jul 11, 2016

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

@ghedamat ghedamat merged commit eb22949 into master Jul 11, 2016
@ghedamat ghedamat deleted the plugin-control branch July 11, 2016 14:08
@lukemelia
Copy link
Contributor
lukemelia commented Jul 11, 2016

🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆

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

Reviewers

No reviews

Assignees

@lukemelia lukemelia

@achambers achambers

Labels

current

Projects

None yet

Milestone

1.0.0

Development

Successfully merging this pull request may close these issues.

Make it easier to disable plugins for certain deploy targets

5 participants

@ef4 @lukemelia @duizendnegen @ghedamat @achambers
0