Skip to content
This repository has been archived by the owner on Jul 31, 2021. It is now read-only.

feat(migrations): add support for migrations #97

Merged
merged 8 commits into from
Jul 8, 2020

Conversation

CriGoT
Copy link
Contributor

@CriGoT CriGoT commented Jun 26, 2020

✏️ Changes

Adds support for the new migrations endpoint. This is expected to be passed in the migrations property as a set of properties of type boolean.

When updating migrations the following checks are performed:

  • The migration flags being set are compared with the migration flags available for the tenant at the moment of processing the import (code).
  • If there are any migration flags that are not available. The behavior will depend on the value set for the flag:
    • If the flag value is set to false, meaning that the migration is being disabled, we will log a warning and ignore the flag.
    • if the flag value is set to true, meaning the migration is still enabled, it will be sent to the server.

The goal of the logic is that the flag van be disabled via the deployment tools and avoid causing errors when the migration period ends.

Additionally a configuration parameter AUTH0_IGNORE_UNAVAILABLE_MIGRATIONS allows to ignore all migrations flags not available regardless of its value. In this case we will log an info message with the names of the flags ignored.

🔗 References

auth0/node-auth0#503

🎯 Testing

🚫 This change has been tested in a Webtask

✅ This change has unit test coverage

🚫 This change has integration test coverage

🚫 This change has been tested for performance

🚀 Deployment

🚫 This can be deployed any time

⚠️ This has be completed with the version bumps in node-auth0

@codeclimate
Copy link

codeclimate bot commented Jun 26, 2020

Code Climate has analyzed commit a56f98d and detected 0 issues on this pull request.

View more on Code Climate.

src/auth0/handlers/migrations.js Show resolved Hide resolved
src/auth0/handlers/migrations.js Outdated Show resolved Hide resolved
src/auth0/handlers/migrations.js Outdated Show resolved Hide resolved
src/auth0/handlers/migrations.js Show resolved Hide resolved
src/auth0/handlers/migrations.js Outdated Show resolved Hide resolved
src/auth0/handlers/migrations.js Outdated Show resolved Hide resolved
@@ -19,7 +19,7 @@ export const schema = {
properties: {
name: { type: 'string', enum: supportedPages },
html: { type: 'string', default: '' },
url: { type: 'string', default: '' },
url: { type: 'string' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was changed because the default was adding a url property in dump that was not expected in deploy-cli. Not related to the PR changes

Copy link

@luuuis luuuis left a comment

Choose a reason for hiding this comment

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

LGTM overall, some minor suggestions. The package-lock.json I think needs fixing.

package-lock.json Outdated Show resolved Hide resolved
src/auth0/handlers/migrations.js Outdated Show resolved Hide resolved
Comment on lines +40 to +44
await stageFn.apply(handler, [ {
migrations: {
migration_flag: false
}
} ]);
Copy link

Choose a reason for hiding this comment

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

I'd use Function.call to avoid the extra array.

Suggested change
await stageFn.apply(handler, [ {
migrations: {
migration_flag: false
}
} ]);
await stageFn.call(handler, {
migrations: {
migration_flag: false
}
});

What is preventing you from using handler.processChanges? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is anything preventing it but instead the desire to mimic the actual call made by the main class https://github.com/auth0-extensions/auth0-source-control-extension-tools/blob/master/src/auth0/index.js#L36-L40

I can clean up to simplify but I was just following the convention of the repo instead of adding a new one.

Copy link

Choose a reason for hiding this comment

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

Fair enough, if that is the convention then let's not change it in this PR.

tests/auth0/handlers/migrations.tests.js Outdated Show resolved Hide resolved
@CriGoT CriGoT marked this pull request as ready for review July 3, 2020 12:10
@faroceann
Copy link
Contributor

LGTM

@faroceann faroceann merged commit 6464e5f into auth0-extensions:master Jul 8, 2020
@CriGoT CriGoT deleted the add-migrations branch July 8, 2020 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants