-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
…ow where the installation is coming from
}), | ||
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.
50fa538
to
7132701
Compare
import { ModeOption } from '../../types/options.js' | ||
import { ROOT_PACKAGE_JSON } from '../json.js' | ||
|
||
const configVersion = ROOT_PACKAGE_JSON.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.
nit: This variable feels unnecessary.
"description": "Netlify config module", | ||
"type": "module", | ||
"exports": "./lib/index.js", | ||
"exports": { |
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.
Did you mean to keep this change?
🎉 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)