Adding vault content inclusion exclusions #962
Conversation
…the terraform exclusion
updating local branch
updating local branch
… folder and wont be reflected otherwise
Vercel Previews Deployed
|
Broken Link CheckerNo broken links found! 🎉 |
|
|
||
| This content should always appear. | ||
|
|
||
| <!-- BEGIN: VLT:>=v1.21.x --> |
There was a problem hiding this comment.
Let's make sure not to include this file when we ship this PR.
scripts/prebuild/mdx-transforms/exclude-terraform-content/index.test.mjs
Outdated
Show resolved
Hide resolved
scripts/prebuild/mdx-transforms/exclude-vault-content/index.test.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Let's created a shared.mjs file to keep some of the duplication down and keep the remark transforms contained.
https://github.com/hashicorp/web-unified-docs/pull/962/files#r2361077992
|
Does the inclusion work for partials? |
@schavis Hey so from looking at the code, content is excluded before partials are processed, so this would mean that it currently doesn't work for partials- however I'll work to change that |
| await expect(async () => { | ||
| return await customRunTransform(markdown, vaultVersion, filePath) | ||
| }).rejects.toThrow( | ||
| 'Directive block CONSUL:>=v1.15.x could not be parsed between lines 2 and 4', |
There was a problem hiding this comment.
I wonder if it might be better to do a regex match against the error thrown rather than an exact string match. Just to make the test more robust. That way if the error message changes for casing or punctuation, it won't fail.
…ils to make overall intention and extensibility clearer
| const directive = rest.join(':') // Handle edge cases like "TFEnterprise:only name:something" | ||
|
|
||
| // Explicit routing | ||
| if (product === 'Vault') { |
There was a problem hiding this comment.
Yeah doing it in separate ones is likely the way forward here. 😢
Let's move this to a dictionary function call though to clean up the code. e.g.
const directiveProcessingFuncs = {
Vault: processVaultBlock,
TFC: processTFCBlock,
TFEnterprise: processTFEnterpriseBlock,
}
directiveProcessingFuncs[product](directive, block, tree, options)
There was a problem hiding this comment.
this change sounds good
@williamdalessandro it looks like that was done on purpose, by me; https://github.com/hashicorp/web-unified-docs-internal/pull/237/commits/c2c9dd65a7e699a9877c63a5d0e2b230d3a0bb6e. I can't exactly remember why though as it was during a push to get some TFE documentation out the door. I dug through a bunch of slack chat logs and this was the closest I could find that might give a hint here. Overall what I am saying is that I am okay with changing it, but let's add some tests for the partial then remove content Plugin workflow. In the slack conversation it looks like a bunch of conversation is around this page: https://github.com/hashicorp/web-unified-docs/blob/main/content/terraform-docs-common/docs/cloud-docs/api-docs/index.mdx. So let's base our partial then remove content Plugin workflow tests around that page. |
…s behavior with global partials and content exclusion
…ere not correctly removed after being placed
…cases I found, simplified ast removal search
| // This ensures exclusion directives in global partials are properly evaluated | ||
| .use(remarkIncludePartialsPlugin, { partialsDir, filePath }) | ||
|
|
||
| // Only apply content exclusion if this is NOT a global partial |
There was a problem hiding this comment.
@williamdalessandro are we sure about this? It sounds like they want content exclusion in global partials: https://app.asana.com/1/90955849329269/task/1211058606978186/comment/1211471674451585?focus=true.
Double check with Sarah here; as I am not 100% sure on this.
There was a problem hiding this comment.
Let's ask Sarah for a test case here, so we can double check that we understand their needs.
There was a problem hiding this comment.
Everything else in this PR looks great. 🎉
We just need to double check this before approval.
Links
Preview Link showing the response of the test page post exclusion
Asana task
What
This is an implementation of version based directives specifically for Vault- differs from the TFC and TFEnterprise exclusions because those were simplified. Does accurate parsing of the version numbers along with relevant error handling/checks. Created new test suite for the new vault exclusions and added in new test to terraform exclusions
Why
Requested in the above asana ticket
How
Extends upon the existing script and copies the same format however it has unique checks since the exclusion operation is more involved. Made the code look identical so it can be easier to abstract in the future so that any product can have access to inclusions/exclusions, right now it just gets the job done.
Pics
The exclusion file
The result when ran
Testing
Can run
and
in terminal to see the tests run for these. Added in a testing file
to see inclusions/exclusions at work. Will remove that file from the PR but want it around for testing reference.
Can spin up locally and see the page at
or
Other info
Had to recompile the binaries since these are changes that take place in the scripts folder
Things left to do:
The following files need to be updated to account for vault exclusions
build-mdx-transforms-file.mjs - Added in and tests work
build-mdx-transforms-file.test.mjs (No change necessary here)
remove this testing file once the PR is deemed as good to go
Create a new ticket proposing a new structure change to make this more extensible with a plugin based OOP architecture