-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Upgrade Ember data 4.11.x to 4.12.x #25272
Conversation
CI Results: |
Build Results: |
ui/mirage/handlers/sync.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be resolved after we upgrade mirage, but that's another PR.
@@ -75,7 +75,7 @@ module('Acceptance | auth', function (hooks) { | |||
let included; | |||
if (backend.type === 'token') { | |||
keys = lastRequest.requestHeaders; | |||
included = 'X-Vault-Token'; | |||
included = 'x-vault-token'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this was changed to all lower case? Also, does this only happen in test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a test thing. and I wish i knew the answer. in a previous ember upgrade it changed to uppercase and here we're changing it back. 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for tackling this upgrade - there were a ton of new ember improvements in this minor release!
@@ -134,6 +134,7 @@ module('Acceptance | mfa-login-enforcement', function (hooks) { | |||
assert.dom('h1').includesText(enforcement.name, 'Name renders in title'); | |||
assert.dom('h1 svg').hasClass('flight-icon-lock', 'Lock icon renders in title'); | |||
assert.dom('[data-test-tab="targets"]').hasClass('active', 'Targets tab is active by default'); | |||
await waitFor('[data-test-target]', { timeout: 5000 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strange that we need to do this, and I'm wondering if it's because we're using @model
in the route template, which is known to cause some issues. Maybe try using this.model
in app/templates/vault/cluster/access/mfa/enforcements/enforcement/index.hbs
and removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chelshaw no luck. Though I did learn something new. Removing the waitFors I always hit the timeout issues even with the change you suggested.
Upgrading Ember data as a step closer to upgrading ember to 5.x.x
Special blog release for 4.12.x.
Label on Ember data's GH that will help us track codemods, etc for upgrading to 5.x.
From the Ember blog, a note about support for ember data 4.12.x
TODO:
Note:
yarn run test:quick
on main, the same tests fail there as do on this branch. Making this note for posterity so folks don't think changes here are responsible for those failures.