-
Notifications
You must be signed in to change notification settings - Fork 139
Replacements deploy #748
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
Replacements deploy #748
Conversation
…source-deploy-retrieve into replacements-deploy
src/convert/metadataConverter.ts
Outdated
const conversionPipeline = pipeline( | ||
new ComponentReader(components), | ||
Readable.from(components), | ||
!targetFormatIsSource && (process.env.DEBUG_REPLACEMENTS_VIA_CONVERT === 'true' || output.type === 'zip') |
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.
want to keep this env var in?
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'm leaning toward yes. It would also enable a use case I don't want to officially encourage (do replacements to create a mdapi folder that you can mdapi deploy)
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.
but I renamed it to match sf
style
{ | ||
"name": "componentSetCreate", | ||
"duration": 644.1402209999505 | ||
"duration": 702.3725049999775 |
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.
should we add a replacement perf test?
src/convert/replacements.ts
Outdated
// used during replacement stream to limit warnings to explicit filenames, not globs | ||
singleFile: Boolean(r.filename), | ||
// Config is json which might use the regex. If so, turn it into an actual regex | ||
toReplace: r.stringToReplace ?? new RegExp(r.regexToReplace, 'g'), |
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.
Line 57 above is looking at toReplace checking if it is a string and if so creating a regex from the string. If toReplace needs to be a regex, why not convert the string into the regex here?
*/ | ||
const readReplacementsFromProject = async (): Promise<ReplacementConfig[]> => { | ||
const proj = await SfProject.resolve(); | ||
const projJson = (await proj.resolveProjectConfig()) as { replacements?: ReplacementConfig[] }; |
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.
@mshanemc is your work going to include updating sfdx-project.schema.json in @salesforce/schemas?
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.
it should. good call.
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 made a WI for it
| { replaceWithEnv?: never; replaceWithFile: string }; | ||
|
||
type ReplacementTarget = | ||
| { stringToReplace: string; regexToReplace?: never } |
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.
Why carry a string and a regex, when a single property would suffice?
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.
exact-match-on-strings is simpler, less error-prone, and require no escaping beyond what json requires for \
considering the common scenarios like emails and urls, user@company.com
or https://foo.com/api
, it's gonna result in more success for users.
import { matchingContentFile } from '../mock'; | ||
import * as replacementsForMock from '../../src/convert/replacements'; | ||
|
||
describe('file matching', () => { |
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.
Given that regexes are loaded from json, there will be nothing that verifies regexes while modifying the project json, allowing an invalid regex to be saved. At runtime the, the regex will fail parse. How will this error bubble out with enough information to properly identify the source of the error?
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.
ERROR running force:source:deploy: Component conversion failed: Invalid regular expression: /BOOM\/: \ at end of pattern
How's that?
* chore: auto-update metadata coverage in METADATA_SUPPORT.md * test: use filenames from mocks * test: record perf Co-authored-by: svc-cli-bot <svc_cli_bot@salesforce.com>
…source-deploy-retrieve into replacements-deploy
QA NotesReplace with ENV VAR
✅ : confirmed in the org string was replaced with ENV VAR value
😐 : in output of Optionally Replace
✅ : nothing replaced when Replace with File
✅ : replaced string with file contents Replace with RegEx
✅ : replaced Replace String with File
✅ : replaces all of the matching strings with content of file Replace String with Env Var
✅ : replaces the string with env var |
QA responses:
globs are not regex.
yeah, because these are streamed at the component level, there's not some top-level context where the stream is concerned with "has there ever been any file that matched this?" It's possibly, but not elegantly.
it's going to be in the docs. |
What does this PR do?
string replacements during deploy
define file via glob or filename
define what to replace via exact strings or regex
define new text from env or file
replacements could happen conditional based on one or more envs
provide output in DeployResponse for plugins to use for display
What issues does this PR fix or reference?
@W-11949818@