Skip to content
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

DXCDT-388: Enabling preservation of array replace markers in YAML #760

Merged
merged 47 commits into from
Mar 3, 2023

Conversation

willvedd
Copy link
Contributor

@willvedd willvedd commented Mar 2, 2023

🔧 Changes

One of the final pieces for delivery a MVP deliverable for keyword preservation on export is enabling the preservation of array replace keywords (ex: @@LOGOUT_URLS@@) in YAML configuration formats. The hurdle with this limitation can be broken down into two major problems:

Problem Statement 1:
When using array replace syntax in a YAML configuration file, the markers can be expressed as-is without quotes, which is invalid YAML. There is a built-in YAML to JSON conversion step that will fail. Example of an error-causing configuration:

tenant:
  allowed_logout_urls: @@LOGOUT_URLS@@

Solution:
Introduce an intermediate step before the YAML parsing that looks for unquoted array replace markers and upon finding them, wrap in quotes. Array replace markers that are already wrapped in quotes should be left as-is.


Problem Statement 2:
When using array replace syntax in a YAML configuration file, the markers can be expressed as-is without quotes, wrapped with double quotes or wrapped in single quotes. While technically always true, the keyword replacement process preserves these array values wrapped in single-quotes.

For example, all of the following are valid configuration files:

tenant:
  allowed_logout_urls: @@LOGOUT_URLS@@
tenant:
  allowed_logout_urls: '@@LOGOUT_URLS@@'
tenant:
  allowed_logout_urls: "@@LOGOUT_URLS@@"

Solution:

Appropriately handle each scenario during the keyword replacement step. In practice, this means introducing a third regex replacement step for single-quoted strings in addition to the double-quoted and unquoted array replace markers.


Together, the combination of these solutions enabled a seamless experience for users needing to preserve array replace keywords in the YAML format.

📚 References

Related RFC: #688

🔬 Testing

Added new unit tests for any functions that were altered. Additionally, added a couple assertions to E2E tests. Finally, did quite a bit of manual testing for another layer of verification.

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

willvedd and others added 30 commits February 21, 2023 15:35
…ort' into fix-make-keyword-preservation-more-flexible
@willvedd willvedd requested a review from a team as a code owner March 2, 2023 23:40
@willvedd willvedd marked this pull request as draft March 2, 2023 23:40
@@ -86,7 +86,8 @@ async function dump(context: YAMLContext): Promise<ParsedActions> {
runtime: action.runtime,
dependencies: action.dependencies || [],
status: action.status,
secrets: mapSecrets(action.secrets || []),
secrets:
typeof action.secrets === 'string' ? action.secrets : mapSecrets(action.secrets || []), //Enables keyword preservation to operate on action secrets
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most array preservation will work as-is for all YAML handlers, but there is this one instance where a string value for action.secrets would trigger a runtime error when applying a map function.

@@ -104,7 +109,7 @@ export default class YAMLContext {

// Run initial schema check to ensure valid YAML
const auth0 = new Auth0(this.mgmtClient, this.assets, toConfigFn(this.config));
await auth0.validate();
if (!opts.disableKeywordReplacement) await auth0.validate();
Copy link
Contributor Author

@willvedd willvedd Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema validation needs to be disabled during keyword-preserved export because a field may be enforced as an array but will be expressed with an array replace marker (string).

Just now realizing that this should be a comment.

Comment on lines +11 to +15
//YAML format supports both single and double quotes for strings
const patternWithSingleQuotes = `'${pattern}'`;
const patternWithDoubleQuotes = `"${pattern}"`;

return new RegExp(`${patternWithQuotes}|${pattern}`, 'g');
return new RegExp(`${patternWithSingleQuotes}|${patternWithDoubleQuotes}|${pattern}`, 'g');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array replace markers could be wrapped in single quotes, double quotes or not wrapped at all. When performing this replacement, it is necessary to check for all three.

// wrapArrayReplaceMarkersInStrings will wrap array replacement markers in quotes.
// This is necessary for YAML format in the context of keyword replacement
// to preserve the keyword markers while also maintaining valid YAML syntax.
export function wrapArrayReplaceMarkersInStrings(body: string, mappings: KeywordMappings): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does the heavy lifting in this PR. It solves the problem outlined in Problem 1 of the description.

Comment on lines +544 to +555
const localAssets = `
tenant:
friendly_name: "##ENV## Tenant"
enabled_locales: @@LANGUAGES@@
connections:
- name: connection-1
strategy: waad
options:
tenant_domain: "##DOMAIN##"
`;

fs.writeFileSync(tenantFile, localAssets);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not possible to test this scenario with JSON, needs to be converted to YAML. Note the unwrapped @@LANGUAGES@@ array replace instance.

@willvedd willvedd marked this pull request as ready for review March 3, 2023 00:04
@willvedd willvedd enabled auto-merge (squash) March 3, 2023 00:04
@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2023

Codecov Report

Patch coverage: 94.44% and project coverage change: +0.04 🎉

Comparison is base (bdc06aa) 84.08% compared to head (f5289db) 84.13%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
+ Coverage   84.08%   84.13%   +0.04%     
==========================================
  Files         115      115              
  Lines        3512     3523      +11     
  Branches      670      671       +1     
==========================================
+ Hits         2953     2964      +11     
  Misses        322      322              
  Partials      237      237              
Impacted Files Coverage Δ
src/context/yaml/handlers/actions.ts 75.00% <0.00%> (ø)
src/context/yaml/index.ts 85.26% <100.00%> (ø)
src/tools/index.ts 100.00% <100.00%> (ø)
src/tools/utils.ts 96.06% <100.00%> (+0.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@willvedd willvedd merged commit 025193b into master Mar 3, 2023
@willvedd willvedd deleted the DXCDT-388-preserve-array-syntax-markers-in-yaml branch March 3, 2023 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants