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

UI: Always add the active region as a query param to API requests #8477

Merged
merged 7 commits into from
Jul 21, 2020

Conversation

DingoEatingFuzz
Copy link
Contributor

Fixes #7309 #8195

This was a straightforward fix but it shone a light on a pretty big test coverage gap. As a consequence, most of this diff is new tests and test refactoring.

Root issue
The root issue is a sorta clumsy intersection of a few truths and a lack of testing on our part.

  1. To make cross-region API requests, the url requested must always include the region query parameter.
  2. Using query params with PUT, POST, and DELETE requests is unorthodox. Typically if a request has a request body, it doesn't also have query params.
  3. Given this convention, Ember (first by way of jquery, now by way of backwards compatibility) assumes anything passed in as data will be encoded as query params for GET requests and as requestBody properties for all other requests.
  4. Without our own intervention, regions were being encoded in the requestBody erroneously instead of as a query param.

Resolution
All API requests going through Ember Data eventually hit the ajaxOptions method of the application adapter. This is already where the region param handling was happening. Now instead of stuffing the property in the data hash, it is manually appended to the URL as a query param.

All API requests going through the token service were unaffected since the region was already manually being added as a query param.

To ensure this is true everywhere and to set a precedent going forward, unit tests for all adapter actions now include variations for use with a non-default region. This includes

  1. Job
    1. fetchRawDefinition
    2. forcePeriodic
    3. stop
    4. parse
    5. plan
    6. run
    7. update
    8. scale
  2. Allocation
    1. stop
    2. restart
    3. `ls
    4. stat
  3. Deployment
    1. promote
  4. Node
    1. setEligible
    2. setIneligible
    3. drain
    4. forceDrain
    5. cancelDrain

@github-actions
Copy link

github-actions bot commented Jul 21, 2020

Ember Asset Size action

As of 544d941

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +103 B +41 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Jul 21, 2020

Ember Test Audit comparison

master 544d941 change
passes 1290 1315 +25
failures 0 0 0
flaky 0 0 0
duration 6m 00s 130ms 6m 30s 159ms +30s 029ms

Copy link
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

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

I have a couple of minor comments but nothing blocking. I didn’t run this locally but I did check it out on Netlify with mirage-logging=true and all looks well 😀

const region = this.get('system.activeRegion');
if (region) {
options.data.region = region;
url = associateRegion(url, region);
Copy link
Contributor

Choose a reason for hiding this comment

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

A nice clean intervention! 😍

@@ -9,9 +9,16 @@ module('Unit | Adapter | Node', function(hooks) {

hooks.beforeEach(function() {
this.store = this.owner.lookup('service:store');
this.system = this.owner.lookup('service:system');
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite minor but it looks like this isn’t accessed anywhere in this test 🤓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

testCases.forEach(testCase => {
test(`setEligible makes the correct POST request to /:node_id/eligibility ${testCase.variation}`, async function(assert) {
const { pretender } = this.server;
if (testCase.region) window.localStorage.nomadActiveRegion = testCase.region;
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure this would work but I wonder if this repeated setup step could be extracted with something like this inside testCases.forEach:

if (testCase.region) {
  hooks.beforeEach(function() {
    window.localStorage.nomadActiveRegion = testCase.region;
  });
}

Maybe it’s not worth it though, since there would still be other boilerplate-ish things shared between tests.

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 tried this and it did not work. I think since tests can run in any order, the only way to make this work is to give each test case its own module...but I'm gonna avoid the nerd snipe 😅

@DingoEatingFuzz DingoEatingFuzz force-pushed the b-ui/cross-region-alloc-lifecycle-buttons branch from 01c5c4e to 544d941 Compare July 21, 2020 16:02
@DingoEatingFuzz DingoEatingFuzz merged commit 248e844 into master Jul 21, 2020
@DingoEatingFuzz DingoEatingFuzz deleted the b-ui/cross-region-alloc-lifecycle-buttons branch July 21, 2020 16:21
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI] Cross-region allocation lifecycle button stop & restart fail with 404
2 participants