-
Notifications
You must be signed in to change notification settings - Fork 71
feat: add header to installing extension with the build mode so we know where the installation is coming from #6478
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
base: main
Are you sure you want to change the base?
Conversation
…ow where the installation is coming from
@@ -32,6 +35,7 @@ export const installExtension = async ({ | |||
}), | |||
headers: { | |||
'netlify-token': netlifyToken, | |||
'User-Agent': extensionInstallSource, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the user agent will be 'buildbot' | 'cli' | 'require'
? For future-proofness, do we want to be a bit more descriptive and include the fact that we're calling from Netlify Build, potentially including the version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yeah, it's not always netlify build though, could be the call to netlify config they do (we still have the double call thing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest something like:
User-Agent: Netlify Build (mode:${extensionInstallationSource}) / ${buildVersion}
If there's any other arbitrary metadata that might be useful to us later in analysis, we can include it semicolon-delimited in the parenthetical.
import { EXTENSION_API_BASE_URL } from '../../integrations.js' | ||
import { ModeOption } from '../../types/options.js' | ||
|
||
const require = createRequire(import.meta.url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to unify this with how we're doing it in build and avoid relying on a CJS feature:
build/packages/build/src/utils/json.ts
Lines 14 to 18 in 750c4b3
export const importJsonFile = async function (filePath: string): Promise<PackageJson> { | |
const fileContents = await readFile(filePath, 'utf-8') | |
return JSON.parse(fileContents) | |
} |
"exports": "./lib/index.js", | ||
"exports": { | ||
".": "./lib/index.js", | ||
"./package.json": "./package.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this so that you can require the package name and not a relative path to the JSON file?
🎉 Thanks for submitting a pull request! 🎉
Summary
This adds a user agent header to our installation of auto-installable extensions.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)