Skip to content

Conversation

mothslaw
Copy link
Contributor

@mothslaw mothslaw commented Jul 23, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • [ X ] Tests for the changes have been added (for bug fixes / features)
  • [ X ] Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • [ X ] Build was run locally and any changes were pushed
  • [ X ] Lint has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • [ X ] Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

No ability for platform plugins to specify whether they want "extended" start/stop hooks

Issue Number: 228

What is the new behavior?

Plugins can optionally specify the new extendedStartStopHooks flag in their plugin config file. If they do not specify, it defaults to false.

Note that there is nothing special that is needed for plugin upgrade here. A plugin could, theoretically, toggle this flag back and forth arbitrarily from release to release (that'd likely be bad for the plugin's users, but it would not cause a problem for the engine or the plugin code)

I did manual testing by building plugins with this flag missing, set to false and set to true. Confirmed the expected values by looking inside artifact.json after the build. Confirmed that the plugin hooks behaved as expected on a live engine.

Does this introduce a breaking change?

  • Yes
  • [ X ] No

Other information

Copy link
Contributor

@nhlien93 nhlien93 left a comment

Choose a reason for hiding this comment

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

looks good, one comment that isn't related to your changes just something I noticed.

'rootSquashEnabled': True,
'buildNumber': '2',
'luaName': 'lua-toolkit-1',
'extendedStartStopHooks': False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not your change at all but can you quickly check why this basic_artifact_content exist outside of the artifact_content fixture? seems odd to me that we need to have two fixtures for the same thing when it's pretty easy to parameterized them to have the information we'd want/need? (you don't need to fix anything if it might take long, but maybe file an issue to track cleaning this up if it is needed?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I can't immediately find any reason for this to exist either. It was added here http://reviews.delphix.com/r/48675/ before this was open-sourced. Neither the review nor the Jira ticket mentions anything about why this was required, instead of just using artifact_content. I filed #241 for this

@mothslaw mothslaw merged commit cc91983 into delphix:develop Jul 29, 2020
mothslaw added a commit to mothslaw/virtualization-sdk that referenced this pull request Jul 29, 2020
Merge pull request delphix#239 from mothslaw/develop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

dvp build should support "start hooks run at enable time" flag
3 participants