Repository navigation
Staging banner linking to prod, and noindex until launch - #23
Conversation
Every non-prod deployment shows 'This is an internal testing build. Use the released tool at <prodUrl>.' The chart's runtime config carries environment (local/staging/prod) and prodUrl, generated per environment by app:sync and passed through the existing envsubst path into envvars.json. The banner shows only when environment is set and isn't prod, and also adds a runtime robots noindex, since staging and prod share one image. app.yaml gains launched (false): until it's true, app:sync puts <meta name="robots" content="noindex"> in src/index.html so the dark- launched prod site isn't indexed. A plain banner until the design system has one. Existing apps unchanged. Closes #14
matthewberry
left a comment
There was a problem hiding this comment.
Review of ed4974a.
Verified
- Checks.
test:scriptspasses 17/17,ng testpasses 33/33 (ChromeHeadless), andapp:sync --checkpasses. I didn't rerunng buildor the screenshot. - An old deployment can't show the banner on prod.
entrypoint.shruns plainenvsubst, which substitutes every variable. I checked locally that an unsetENVIRONMENTbecomes"", so a new image on an old deployment getsenvironment: ""and shows nothing. The config is in place before the component's constructor runs:main.tsloadsenvvars.jsoninprovideAppInitializer. The configmap renders all of.Values.config(toYaml), so the new keys reach it without template changes. - The index.html rewrite (probed with
updateIndexHtml):- unlaunched, it adds one tag at the
<title>'s indentation; - re-syncing is byte-identical;
launched: truerestores the original exactly;- CRLF files stay stable with one tag.
- unlaunched, it adds one tag at the
- Overlap with #22 (head fe3d8ad). A trial merge conflicts only in
scripts/app-sync.test.mjs, two hunks: the fixture tests' image handling vs.withoutBannerConfig. That matches the PR's note. Whichever PR merges second keeps both changes.
Findings (inline)
launched: trueleaves any differently writtennoindextag in place, so the launched site silently stays out of search (app-sync.mjs:209). Reproduced.- Note: the new
configMapKeyRefs aren'toptional(deployment.yaml:60). Not reproduced.
Note: after launch, staging's only noindex is the one added at runtime. Once launched: true removes the static tag, staging is excluded from search only by the Meta tag, which appears after Angular boots. Crawlers that don't run JavaScript would index staging. That may be acceptable, since staging is internal, but it's worth knowing.
Didn't investigate: the banner's look or its contrast in the design system, and chart rendering with helm.
| if (!/<title>[^<]*<\/title>/.test(html)) throw new Error('src/index.html has no <title>'); | ||
| // A function, so `$&` and friends in the name are inserted literally | ||
| return html.replace(/<title>[^<]*<\/title>/, () => `<title>${escaped}</title>`); | ||
| const NOINDEX = /\n[ \t]*<meta name="robots" content="noindex" \/>/; |
There was a problem hiding this comment.
launched: true only removes the exact tag that sync wrote. Any other robots noindex tag survives, so a launched site can stay out of search results without anyone noticing. (Reproduced.) NOINDEX matches only <meta name="robots" content="noindex" />. In a probe with launched: true, each of these was left in index.html, and re-syncing kept it, so --check passes:
<meta name="robots" content="noindex">(no self-closing slash)<meta name="robots" content="noindex, nofollow" /><meta content="noindex" name="robots" />
A hand edit like adding nofollow is the obvious way to end up there, and the result is a site that has launched but never appears in search. Possible fix: when launched is true, throw if index.html still contains any <meta name="robots" …noindex…> that the regex can't remove. That's the same "fail rather than skip" rule #21 adopted for tags.
Minor, same regex: if two copies of the exact tag exist, re-syncing keeps both (stable at 2). That's harmless.
There was a problem hiding this comment.
Reproduced with your three variants, then fixed in d286b76. After removing its own tag, app:sync now refuses any remaining <meta name="robots">, launched or not, and says indexing is controlled by launched. A test covers all three variants in both states.
| name: {{ .Release.Name }}-config | ||
| key: frontendOnly | ||
|
|
||
| - name: ENVIRONMENT |
There was a problem hiding this comment.
Note (not reproduced; I have no cluster or helm here). These configMapKeyRefs aren't optional. If the new chart template is deployed with values that lack environment or prodUrl, for example an existing app's own values or a separate deploy repo, the pod fails to start (CreateContainerConfigError). It wouldn't fall back to "unset = no banner". In this repo the template and the generated values always change together, and the existing keys follow the same pattern, so I'm not raising it as a defect. optional: true on the two new keys would make the "unset shows nothing" design hold for the chart as well as for the image.
There was a problem hiding this comment.
Agreed. Both refs are optional: true in d286b76. An unset key becomes "" through envsubst, which shows no banner.
…onal Review of #23 (5579435029): - Reproduced: with launched: true only the exact generated noindex tag was removed, so a variant (no slash, 'noindex, nofollow', attributes swapped) survived and --check passed while the site stayed out of search. Any robots meta app:sync didn't write is now an error, launched or not. - environment/prodUrl configMapKeyRefs are optional, so a config without them means no banner (unset becomes '' through envsubst) rather than CreateContainerConfigError.
|
Re the note in 5579435029 (staging after launch relies on the runtime tag only): leaving it as a known limitation. A crawler-proof version needs an |
matthewberry
left a comment
There was a problem hiding this comment.
Re-review of d286b76. Nothing blocking.
- The robots variants are fixed. All three variants I reported now throw, and so do two copies of the exact tag (previously stable at 2).
test:scriptspasses 18/18, and--checkpasses on the clean tree. One optional edge case is inline. optional: trueis on both newconfigMapKeyRefs. I didn't render the chart (nohelmhere).- Staging after launch. Leaving it as a documented limitation is fine. The
add_headerinteraction with #7's per-location headers is a good reason to keep it out of this PR. - CI was still running on this head when I checked, so I haven't seen the result.
| // Functions, so `$&` and friends in the name are inserted literally | ||
| let out = html.replace(NOINDEX, () => ''); | ||
| // Any other robots tag would survive launch and keep the site out of search | ||
| if (/<meta\b[^>]*\bname=["']robots["']/i.test(out)) { |
There was a problem hiding this comment.
Optional, low (reproduced): the guard requires a quoted name, so <meta name=robots content=noindex> (valid HTML) still survives launched: true without an error. So does a crawler-specific <meta name="googlebot" content="noindex" />. Both are unlikely to be typed into a template's index.html. name=["']?(robots|googlebot|bingbot)\b would close them if you want to be thorough. Not blocking.
There was a problem hiding this comment.
Reproduced both, fixed in 805eeef. Detection now matches any <meta> whose name is robots or whose content contains noindex, quoted or not. That covers googlebot, bingbot and any other crawler without listing names. Both are now test cases. Script tests: 18/18.
Review of #23 (5579446194), reproduced: <meta name=robots content=noindex> and <meta name="googlebot" content="noindex" /> survived launched: true. Detection now matches any meta whose name is robots or whose content contains noindex, quoted or not.
Closes #14.
What
StagingBannerComponent). It's a plain amber strip until the design system has one.environment(local/staging/prod) andprodUrl, generated per environment byapp:sync.deployment.yamlpasses them asENVIRONMENTandPROD_URLthrough the existingenvsubstpath intoenvvars.json.environmentis set and isn'tprod. An unset value, from an old config or before load, shows nothing.envvars.jsonsetsenvironment: localand an emptyprodUrl, so it shows the banner without a link.app.yamlgainslaunched: false. Until it'strue,app:syncputs<meta name="robots" content="noindex">insrc/index.html, so the dark-launched prod site isn't indexed. The banner also adds a runtimenoindexon non-prod, because staging and prod run the same image.Verification
test:scripts17/17. New tests:prodUrl= prod host;noindexpresent exactly once until launch, stable when re-synced, and removed when launched;launchedmust be a boolean.app:sync --checkpasses, and the build passes.Notes
scripts/app-sync.mjsand its tests. This branch is frommain, so whichever lands second needs a small rebase: the fixture tests' image handling from Push frontend images to ghcr (pr-N/staging from PRs, main/sha from main) #22 plus the banner-key stripping from here.EnvironmentServicetest stub now setsenvironment: 'prod', so other components' specs don't render the banner.