Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(nms): Downgrade dependencies to fix NMS #12149

Merged
merged 2 commits into from
Mar 31, 2022

Conversation

sebathomas
Copy link
Contributor

@sebathomas sebathomas commented Mar 16, 2022

Summary

Users have reported errors starting NMS in v1.6 and v1.6.1 that seem to be due to getting incompatible versions of the DB migration dependencies.

See:
#11291
#11278
#11325

Pinning the versions of @fbcnms/platform-server and @fbcnms/sequelize-models resulted in a different error with an organizations query introduced in:
magma/fbc-js-core#108

By downgrading @fbcnms/platform-server and @fbcnms/express-middleware to a version before that change, I am able to start NMS. This will have the unfortunate side effect that organization queries are no longer case-insensitive.

Test Plan

Check out v1.6 or v1.6.1, build NMS according to the Quick Start Guide and try to log in using admin@magma.test.

Additional Information

  • This change is backwards-breaking

NMS was broken on v1.6 due to a bug in the dependencies.

Signed-off-by: Sebastian Thomas <sebastian.thomas@tngtech.com>
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯 Please note that all commits must be signed off. This is enforced by the DCO check.

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@sebathomas
Copy link
Contributor Author

sebathomas commented Mar 16, 2022

More details about the error:
If you don't downgrade the versions, you get the following message when you try to log into NMS:

magmalte-magmalte-1     | 2022-02-11T14:45:55.465Z [auth/express.js] error: Error getting organization Invalid value Where {
magmalte-magmalte-1     |   attribute: Fn { fn: 'lower', args: [ Col { col: 'name' } ] },
magmalte-magmalte-1     |   comparator: '=',
magmalte-magmalte-1     |   logic: Fn { fn: 'lower', args: [ 'magma-test' ] } }

This seems to be a problem with the queries introduced in magma/fbc-js-core#108, e.g.

  return await Organization.findOne({
    where: {
      name: Sequelize.where(
        Sequelize.fn('lower', Sequelize.col('name')),
        Sequelize.fn('lower', subdomain),
      ),
    },
  });

To me, it looks like there should be only one SQL lower in this query but I'm not familiar with Sequelize.

On the other hand, I don't understand why we don't see these issues on Magma master, where we use the same code.

@sebathomas sebathomas marked this pull request as ready for review March 16, 2022 15:00
@sebathomas sebathomas requested review from a team and andreilee March 16, 2022 15:00
@Neudrino
Copy link
Contributor

Neudrino commented Mar 23, 2022

Closing and reopening to rerun stuck CI. There were a lot of issues in March 2022.

Copy link
Contributor

@jheidbrink jheidbrink left a comment

Choose a reason for hiding this comment

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

Approving as I can confirm that the changes resolve the described issue on my machine.

Copy link
Contributor

@andreilee andreilee left a comment

Choose a reason for hiding this comment

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

Could you get yarn.lock update as well? Otherwise this looks good to me!

Signed-off-by: Jan Heidbrink <jan.heidbrink@tngtech.com>
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Mar 28, 2022
@jheidbrink
Copy link
Contributor

I updated yarn.lock and verified again that the containers still build and dev_setup.sh succeeds.

@maxhbr maxhbr requested a review from andreilee March 28, 2022 12:24
@jheidbrink
Copy link
Contributor

@andreilee Some required CircleCI checks are pending. Can you merge this anyway, given that we don't use CircleCI anyore?

@hcgatewood
Copy link
Contributor

Force merging per @andreilee request

@hcgatewood hcgatewood merged commit 81e59f2 into magma:v1.6 Mar 31, 2022
mattymo pushed a commit that referenced this pull request Jul 6, 2022
* fix(nms): Downgrade dependencies to fix NMS

NMS was broken on v1.6 due to a bug in the dependencies.

Signed-off-by: Sebastian Thomas <sebastian.thomas@tngtech.com>

* fix(nms): Also update yarn.lock

Signed-off-by: Jan Heidbrink <jan.heidbrink@tngtech.com>

Co-authored-by: Jan Heidbrink <jan.heidbrink@tngtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
5 participants