Conversation
pajgo
force-pushed
the
feat/cloudflare-access-list
branch
2 times, most recently
from
e69c6e6 to
e2bc1ce
Compare
Codecov Report
@@ Coverage Diff @@
## staging #474 +/- ##
===========================================
+ Coverage 87.38% 88.30% +0.92%
===========================================
Files 149 157 +8
Lines 2759 3002 +243
===========================================
+ Hits 2411 2651 +240
- Misses 348 351 +3
Continue to review full report at Codecov.
|
| 'redisCluster', | ||
| 'http', | ||
| 'prometheus', | ||
| 'consul', |
There was a problem hiding this comment.
this is a breaking change, instead of updating default list of plugins, setup this as a dependency on the cf-worker and its action. if worker or action is enabled during startup, push consul plugin into init configuration
There was a problem hiding this comment.
Added check whether consul plugin included in configuration, for now.
Think we should add a feature that allows us to include plugins in the constructor of the @microfleet/core plugin system?
| const route = `${routes.prefix ? `${routes.prefix}.` : ''}cf.add-to-access-list`; | ||
|
|
||
| if (remoteip !== false && user.metadata[audience].plan !== 'free') { | ||
| await amqp.publish(route, { remoteip }); |
There was a problem hiding this comment.
may want to add confirm, mandatory and deliveryMode flags for amqp, otherwise this may not be published
|
|
||
| class CloudflareAPI { | ||
| constructor(cfClient, config = {}, withProps = {}) { | ||
| assert(cfClient instanceof CloudflareClient, 'invalid client'); |
There was a problem hiding this comment.
this wont allow stubbing during tests, so I'd add a condition that this is not a test environment
There was a problem hiding this comment.
Removed instanceof check, the constructor now checks the existence of parameter.
| apiUrl(t, values) { | ||
| const { accountId } = this.props; | ||
| assert(accountId, 'accountId invalid'); | ||
| return Object.entries({ accountId, ...values }) | ||
| .reduce((prev, [prop, val]) => prev.replace(`:${prop}`, val), t); | ||
| } |
There was a problem hiding this comment.
this is an expensive operation (iterating, searching, replacing) - consider using template literals (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals)
There was a problem hiding this comment.
Changed to template literals + extra Fns.
| return retry( | ||
| async (retryFn) => { | ||
| try { | ||
| const response = await this.http.get(url); |
There was a problem hiding this comment.
this implementation doesnt look good, try this module with retry by default and explicit error that needs to be thrown (pRetry.abort) to abort retries https://www.npmjs.com/package/p-retry
There was a problem hiding this comment.
Changed package + added extra tests
pajgo
force-pushed
the
feat/cloudflare-access-list
branch
from
326f5ce to
711801d
Compare
feat(cf-access-list): Cloudflare Ip List sync worker test(cf-access-list): fix test to ip binding
pajgo
force-pushed
the
feat/cloudflare-access-list
branch
from
b203bd2 to
175153f
Compare
|
🎉 This PR is included in version 14.12.0-rc.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
feat(cf-access-list): Cloudflare Ip List sync worker test(cf-access-list): Fix test to ip binding
* fix: added rc release * fix: added clear cache on create, update organization (#475) * fix: added clear cache on create, update organization * fix: added debug for send invite mail * fix: added remove user from org on user.remove, edit generate email (#476) * fix: added username to query on organization invite mail * fix: added username to query on organization register mail * feat: edit tbits authenticate (#479) * feat(cf-access-list): Cloudflare Ip List API + client (#474) * feat(cf-access-list): Cloudflare Ip List sync worker Co-authored-by: Pavel Rogovoi <51755949+pajgo@users.noreply.github.com>
2 participants
No description provided.