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

Feature/update brylie meteor api umbrella. Closes #591 #619

Merged
merged 7 commits into from
Nov 23, 2015

Conversation

brylie
Copy link
Contributor

@brylie brylie commented Nov 11, 2015

@apinf/developers please review carefully. I have updated the API Umbrella integration plugin, and made changes to our code to use the new interface. Please help test this to avoid breaking changes.

@bajiat
Copy link
Contributor

bajiat commented Nov 12, 2015

@mauriciovieira Would you please review this PR?

@mauriciovieira
Copy link
Contributor

@bajiat yes!

@mauriciovieira mauriciovieira self-assigned this Nov 12, 2015
"apiKey": "xxx",
"authToken": "xxx",
"baseUrl": "https://example.com/api-umbrella/",
"host": "https://example.com"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not also add a settings.json.example file to the project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done. I misunderstood your comment initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I meant 'adding'.

@mauriciovieira
Copy link
Contributor

@bajiat, @brylie, due to apinf/meteor-api-umbrella#5, I think this review has to wait.

@mauriciovieira
Copy link
Contributor

@baijat, @brylie apinf/meteor-api-umbrella#6 should be accepted and the new version installed to api-umbrella-dashboard. Then the application should work smoothly and this PR will be ok. I will test tomorrow.

@@ -23,7 +23,7 @@ blaze-tools@1.0.4
boilerplate-generator@1.0.4
bruz:github-api@0.2.4_1
brylie:accounts-admin-ui@0.3.0
brylie:api-umbrella@0.2.12
brylie:api-umbrella@1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be 1.0.1

@mauriciovieira
Copy link
Contributor

@brylie I am getting an error on syncApiBackends function triggered at startup.

W20151118-09:56:00.442(-2)? (STDERR)
W20151118-09:56:00.444(-2)? (STDERR) /Users/mauricio/.meteor/packages/meteor-tool/.1.1.10.10lkrly++os.osx.x86_64+web.browser+web.cordova/mt-os.osx.x86_64/dev_bundle/server-lib/node_modules/fibers/future.js:245
W20151118-09:56:00.444(-2)? (STDERR)                        throw(ex);
W20151118-09:56:00.444(-2)? (STDERR)                              ^
W20151118-09:56:00.444(-2)? (STDERR) Error: Invalid characters. Only 0-9 characters are allowed.
W20151118-09:56:00.444(-2)? (STDERR)     at getErrorObject (packages/aldeed_collection2/packages/aldeed_collection2.js:425:1)
W20151118-09:56:00.444(-2)? (STDERR)     at [object Object].doValidate (packages/aldeed_collection2/packages/aldeed_collection2.js:408:1)
W20151118-09:56:00.444(-2)? (STDERR)     at [object Object].Mongo.Collection.(anonymous function) [as insert] (packages/aldeed_collection2/packages/aldeed_collection2.js:177:1)
W20151118-09:56:00.444(-2)? (STDERR)     at server/methods/apiBackends.js:15:1
W20151118-09:56:00.444(-2)? (STDERR)     at Array.forEach (native)
W20151118-09:56:00.444(-2)? (STDERR)     at Function._.each._.forEach (packages/underscore/underscore.js:105:1)
W20151118-09:56:00.444(-2)? (STDERR)     at [object Object].Meteor.methods.syncApiBackends (server/methods/apiBackends.js:9:1)
W20151118-09:56:00.445(-2)? (STDERR)     at maybeAuditArgumentChecks (livedata_server.js:1698:12)
W20151118-09:56:00.445(-2)? (STDERR)     at livedata_server.js:1611:18
W20151118-09:56:00.445(-2)? (STDERR)     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
W20151118-09:56:00.445(-2)? (STDERR)     at [object Object]._.extend.apply (livedata_server.js:1610:45)
W20151118-09:56:00.445(-2)? (STDERR)     at [object Object]._.extend.call (livedata_server.js:1553:17)
W20151118-09:56:00.445(-2)? (STDERR)     at server/startup.js:26:1
W20151118-09:56:00.445(-2)? (STDERR)     at /Users/mauricio/Repositories/github.com/apinf/api-umbrella-dashboard/.meteor/local/build/programs/server/boot.js:249:5
=> Exited with code: 8

after meteor reset it is still happening and I don't have a clue of why the collection is not inserting the apiBackend. Maybe it is a data error from the endpoint, but I could not debug since meteor shell is not working because the application does not start.

Are you getting the same error?

@brylie
Copy link
Contributor Author

brylie commented Nov 18, 2015

We can debug by console logging the API Backend instead of inserting it into the database, within the syncApiBackends method.

@brylie
Copy link
Contributor Author

brylie commented Nov 18, 2015

It looks like there is an entry in API Umbrella with a zero for port number, within the servers array:

{ [Error: Invalid characters. Only 0-9 characters are allowed. [400]]
      error: 400,
      reason: 'Invalid characters. Only 0-9 characters are allowed.',
      details: '[{"name":"servers.0.port","type":"regEx","value":"0"}]',
      message: 'Invalid characters. Only 0-9 characters are allowed. [400]',
      errorType: 'Meteor.Error' } }
 =======================
 { backend_host: 'umbrella.apinf.io',
   backend_protocol: 'http',
   balance_algorithm: 'least_conn',
   created_at: Mon Nov 16 2015 12:09:51 GMT+0200 (EET),
   created_by: '29be177c-865b-4dcb-ad2e-49670d7a19ec',
   frontend_host: 'books.googleapis.com',
   name: 'Test_google_books',
   servers: [ { host: 'books.googleapis.com', port: '0' } ],
   settings: 
   { default_response_headers_string: 'Access-Control-Allow-Origin: *',
      override_response_headers_string: 'Access-Control-Allow-Origin: *' },
   sort_order: 29,
   updated_at: Mon Nov 16 2015 12:09:51 GMT+0200 (EET),
   updated_by: '29be177c-865b-4dcb-ad2e-49670d7a19ec',
   url_matches: [ { backend_prefix: '/', frontend_prefix: '/test_api4/g/books' } ],
   version: 1,
   id: '0304b666-9f56-4738-8e09-8e4c94a4f795',
   managerIds: [ null ] }

I am not sure how this was entered into the system, but the issue should resolve if we delete the backend from API Umbrella.

@brylie
Copy link
Contributor Author

brylie commented Nov 18, 2015

Basically, our regExp for port number looks for 2-5 numeric characters, while API Umbrella seems to allow any Numeric value.

@brylie
Copy link
Contributor Author

brylie commented Nov 18, 2015

@elnzv do you recall where you entered this API Backend (API Umbrella or Apinf)? I am trying to figure out how port zero passed validation.

@mauriciovieira
Copy link
Contributor

@brylie ok, if it is a data problem that might happen again, another solution is to treat the error and ignore the faulty apiBackend that comes back from the rails umbrella endpoint. Imagine a client that has an old api-umbrella installation, with buggy endpoints inserted, and want to plug our dashboard on top of it. Is that a impossible corner-case?

@mauriciovieira
Copy link
Contributor

apinf/api-umbrella-dashboard@ef21d86 is what I meant with treat the error

@brylie
Copy link
Contributor Author

brylie commented Nov 20, 2015

We can ignore the faulty api backend, but it should not fail silently. I.e. we need to at least log somewhere that the backend was rejected, so the administrator is aware and can troubleshoot.

@brylie
Copy link
Contributor Author

brylie commented Nov 23, 2015

@mauriciovieira is there any more work needed before this PR can be merged? Is the error treatment taking place in another task?

@mauriciovieira
Copy link
Contributor

@brylie I believe this PR can be merged. #636 fixes it, using console.error to log (by now).

mauriciovieira added a commit that referenced this pull request Nov 23, 2015
…mbrella

Feature/update brylie meteor api umbrella. Closes #591
@mauriciovieira mauriciovieira merged commit 265a38c into develop Nov 23, 2015
@brylie brylie deleted the feature/update-brylie-meteor-api-umbrella branch November 23, 2015 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants