Skip to content

Conversation

@nhlien93
Copy link
Contributor

@nhlien93 nhlien93 commented May 1, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

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

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • 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?

Currently the SDK does not support upgrading from lua toolkits up to a python plugin. It only supports python to python upgrade. There is no way to denote a migration is a different type other than a plugin migration.

What is the new behavior?

The new protobuf changes made in Tom's diff makes the request include an optional string lua_upgrade_version which, if not null, represents the lua version the upgrade is actually from. the SDK takes this lua version and figures out which migrations need to be run based on this version and concats that list with the list of migration ids passed over in the protobuf message. Then we loop through this list and run all the migrations.

The way we expect a plugin author to denote a lua migration is by setting a second parameter in the decorator: MigrationType.LUA. This lua migration will be saved during the dvp build into a different object specifically for lua migrations. The two migration helper objects do slightly different things:
A Platform migration will contain a set that validates all migration ids are unique. The format checked is also different, and we canonicalize the id into a array if ints so the representation is always the same. The platform migration also returns back the ordered list of migration ids so that the artifact's manifest has a list of all migration ids.
A Lua migration will not contain a set because each of the different objects types (repo, source config, linked source, virtual source, snapshot) can potentially have the same version. Instead the validation will make sure that it's in a major.minor format (the same way it's split on the DE). We also canonicalize the version down to two ints (so something like 1.01 -> 1.1). Lastly we get a list of the sorted lua versions starting from the lua version that is passed in. so for example if there were versions 1.2, 2.3, 0.1, 3.4 and we queried this function with 1.2 the function should return back ['1.2', '2.3', '3.4'].

I ran the following tooling to fix formatting:

Format
python -m yapf --recursive --in-place src/main/python
python -m yapf --recursive --in-place src/test/python
Sort imports
python -m isort -rc src/main/python
python -m isort -rc src/test/python
Lint
Python -m flake8 src/main/python
Python -m flake8 src/test/python

Does this introduce a breaking change?

  • Yes
  • No

Testing

Manually tested that a lua toolkit was upgraded to a python plugin and the migrations were actually run so that optional fields that were added in the python plugin was also modified in the real objects.

Add environments, install lua toolkit, and then link and provision.
Attempt to upgrade to python toolkit with correct lua migrations + luaName set but get this:

(venv) delphix@ip-10-110-219-90:~$ dvp upload -e ln-trunk.dlpxdc.co -u admin -a nix_staged_python.json --password delphix --wait
FILE_UPLOAD job started.
Validating schemas for plugin "nix_staged_python".
Upgrading toolkit "nix_staged_python" from version "1.0.0" to "2.1.0" cannot be performed because there are enabled sources.
Disable the following sources and try again: VNix_MCN, Nix Staged SourceConfig.
Failed trying to upload plugin nix_staged_python.json.

Then disable both dsource and vdb.
Attempt to upload to new python plugin again. This time it should go through:

(vsdk) lindsey.nguyen@lnguyen-mbpro:~/Documents/testnewappdata/qa-appdata-toolkits/toolkits/unix/staged/nix_staged_python$ dvp upload -e ln-trunk.dlpxdc.co -u admin -a ~/Desktop/luathings/nix_staged_python.json --password delphix --wait
FILE_UPLOAD job started.
Validating schemas for plugin "nix_staged_python".
Upgrading 2 repositories for plugin "nix_staged_python".
Upgrading 3 source configs for plugin "nix_staged_python".
Upgrading 1 linked sources for plugin "nix_staged_python".
Upgrading 1 virtual sources for plugin "nix_staged_python".
Upgrading snapshots 1 - 2 (out of 2) for plugin "nix_staged_python".
Updating references to inactive plugin "*Nix Staged Toolkit".
Deleting retired plugin "*Nix Staged Toolkit" ("1.0.0").
FILE_UPLOAD job completed successfully.
Plugin nix_staged_python.json was successfully uploaded.

Now enable the the dsource and vdb and verify that it works (I had to remove the asserts to get this to work, but I imagine if I knew what to update the fields to it would have worked fine)

Also added a bunch of unit tests that makes sure the right migrations get run in the right order when the protobuf request has a lua_upgrade_version.

Fixes #71

@nhlien93 nhlien93 marked this pull request as ready for review May 1, 2020 13:00
@ankursarin ankursarin added this to the VSDK Sprint 4/30-5/21 milestone May 6, 2020
@nhlien93 nhlien93 self-assigned this May 6, 2020
@nhlien93 nhlien93 changed the title Make change to allow for LUA type migrations in the upgrade operations Fixes #71 Make change to allow for LUA type migrations in the upgrade operations May 6, 2020
@nhlien93 nhlien93 changed the title Fixes #71 Make change to allow for LUA type migrations in the upgrade operations Make change to allow for LUA type migrations in the upgrade operations May 6, 2020
Copy link
Contributor

@ravi-cm ravi-cm 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. Thanks for the additional changes in fake/test plugins.

@nhlien93 nhlien93 merged commit 0a0e88b into delphix:develop May 8, 2020
@nhlien93 nhlien93 deleted the luaupgrades branch May 21, 2020 03:21
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.

Lua to Python - Make change to allow for LUA type migrations in the upgrade operations

5 participants