feat: different countly keys for kubo and webui.ipfs.io deployments #2081
Conversation
|
Need to update this to reflect lidel's recommendation in #2078 (comment) |
src/env.js
Outdated
| export async function getDeploymentEnv () { | ||
| const origin = globalThis.location.origin | ||
| if (origin.includes('webui-ipfs') || origin.includes('webui.ipfs')) { | ||
| if (origin.includes('localhost')) { | ||
| return 'local' | ||
| } | ||
| return 'webui.ipfs' | ||
| } | ||
| try { | ||
| const response = await fetch('/webui', { method: 'HEAD' }) | ||
| if (response.redirected) { | ||
| /** | ||
| * @see https://github.com/ipfs/kubo/blob/f54b2bcf6061219f7f481e8660e6707ae3bc3c38/cmd/ipfs/daemon.go#L677 | ||
| * @see https://github.com/ipfs/kubo/blob/15093a00116ecf6b550023155f31a33f4bba6403/core/corehttp/webui.go#L57 | ||
| */ | ||
| return 'kubo' | ||
| } | ||
| return 'local' | ||
| } catch { | ||
| return 'webui.ipfs' | ||
| } | ||
| } |
There was a problem hiding this comment.
@lidel how does this look
…etween-webuiipfsio-and-webui-from-kubo
src/env.js
Outdated
| } | ||
| try { | ||
| const response = await fetch('/webui', { method: 'HEAD' }) | ||
| if (response.redirected) { |
There was a problem hiding this comment.
NIT:
is this kubo check necessary? if not local or webui.ipfs.io it would be kubo correct?
is this check just a safeguard for future?
There was a problem hiding this comment.
Unfortunately, it is required as we can't really guarantee that it's kubo unless it meets the expectations of kubo. We should assume it's typical webui unless we can guarantee it's kubo. I added some tests to express this more clearly, but as lidel mentioned at #2078 (comment) it's susceptible to silent failures upon kubo's changing of behavior.
There was a problem hiding this comment.
that question was something i was wondering myself after addressing lidel’s latest comment. but i’d rather have some webui metrics for kubo under webui (when/if they change behavior) than non-kubo webui metrics listed for kubo
There was a problem hiding this comment.
makes sense, tests help too. LGTM
| const countlyAppKeyPromise = pickAppKey() | ||
| countlyAppKeyPromise.then((appKey) => { | ||
| Countly.app_key = appKey | ||
| }) |
There was a problem hiding this comment.
| const countlyAppKeyPromise = pickAppKey() | |
| countlyAppKeyPromise.then((appKey) => { | |
| Countly.app_key = appKey | |
| }) | |
| Countly.app_key = await pickAppKey() |
There was a problem hiding this comment.
👎 this actually fails some existing tests and was how I had the code originally
| if (webuiRegex.test(origin)) { | ||
| return localhostRegex.test(origin) ? 'local' : 'webui.ipfs' | ||
| } |
There was a problem hiding this comment.
why not move this in the try block too?
There was a problem hiding this comment.
Can regex.test actually throw? The only way I can think of is if the object passed to it throws on toString:
var f = /test/
var o = { toString: () => { throw new Error('test') }}
f.test(o)
| return 'kubo' | ||
| } | ||
| } catch { | ||
| // dont throw from this method |
There was a problem hiding this comment.
eh.. i don't see what value we would get from it. This is an expected throw in the majority of usecases (maybe not the majority after we see the metrics)
| testOriginAndEnv('https://webui.ipfs.io', () => {}, 'webui.ipfs', 0) | ||
| testOriginAndEnv('https://webui-ipfs-io.ipns.dweb.link/', () => {}, 'webui.ipfs', 0) | ||
| testOriginAndEnv('https://webui.ipfs.tech', () => {}, 'webui.ipfs', 0) | ||
| testOriginAndEnv('https://some-random-url', () => { throw new Error('no match on origin') }, 'webui.ipfs', 1) | ||
| testOriginAndEnv('https://some-random-url', () => ({ redirected: false, url: 'https://some-random-url/webui' }), 'webui.ipfs', 1) |
There was a problem hiding this comment.
a small improvement could be to use named args (destructured args) where we can have default fetchImpl = () => {}
There was a problem hiding this comment.
yea, I thought about that, but the parameter list is cleaner when writing the tests. I usually default to arg objects as it makes changes easy.. but I'm hoping (save the link to this comment =P) that we won't need to adjust this much.
SgtPooki
deleted the
2078-ensure-we-have-a-way-to-differentiate-between-webuiipfsio-and-webui-from-kubo
branch
## [2.22.0](v2.21.0...v2.22.0) (2023-01-27) CID `bafybeifeqt7mvxaniphyu2i3qhovjaf3sayooxbh5enfdqtiehxjv2ldte` --- ### Features * different countly keys for kubo and webui.ipfs.io deployments ([#2081](#2081)) ([2e766fa](2e766fa)) ### Bug Fixes * import status window UX ([#2075](#2075)) ([4104cc6](4104cc6)) * webui update metrics to opt-out by default (part of 2074) ([#2084](#2084)) ([cac7663](cac7663)) ### Trivial Changes * pull new translations ([#2076](#2076)) ([ad9e4ff](ad9e4ff))
|
🎉 This PR is included in version 2.22.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
fix #2078