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.
|
|
||
| import { PRODUCT_CONFIG } from '#productConfig.mjs' | ||
|
|
||
| export const DIRECTIVE_PRODUCTS = ['VLT', 'TFC', 'TFEnterprise'] |
There was a problem hiding this comment.
@williamdalessandro let's move this into the exclude-terraform-content transform. That logic is specific to there. I think you moved it here so it's "easier to find" but we should keep all of the logic for the transformed self contained.
There was a problem hiding this comment.
Looking over this PR a bit more I think I see why you did this. You separated out the TF/TFE/HCP TF exclusions from the Vault exclusions. In that case I think it should be in it's own file; something like so:
scripts/prebuild
- ...
- exclude-content
- shared.mjs
- exclude-terraform-content.mjs
- exclude-vault-content.mjs
- ...
There was a problem hiding this comment.
I was torn between what to do for this, putting it in its own file sounds good
There was a problem hiding this comment.
@williamdalessandro one more thought here. Let's use Vault instead just VLT. Mainly for the reason that from my understanding we don't never use VLT to mean Vault. Whereas TFC and TFE are both commonly used. It's just a few extra characters that I think will make things a bit clearer for the education team.
| // Check if this is a product we should handle | ||
| const productMatch = flag.match(/^(\w+):/) | ||
|
|
||
| if (productMatch && DIRECTIVE_PRODUCTS.includes(productMatch[1])) { |
There was a problem hiding this comment.
Hmm I like this, I figure you did it to save build time. It definitely seems to work.
"Deploy Project Artifacts and Set GitHub Outputs" for UDR with this change in CI/CD is 8 mins here versus 11-12 mins normally. 🎉
There was a problem hiding this comment.
that's actually an unintended result tbh- this came here as a result of making sure that the terraform exclusion script wouldnt freak out when it saw the vault directive
|
|
||
| import { describe, it, expect, vi } from 'vitest' | ||
| import { transformExcludeTerraformContent } from './index.mjs' | ||
| // import { DIRECTIVE_PRODUCTS } from '../build-mdx-transforms.mjs' |
There was a problem hiding this comment.
Let's remove this code comment if it's not used.
| it('should throw an error for unexpected BEGIN block', async () => { | ||
| const markdown = ` | ||
| <!-- BEGIN: VLT:>=v1.21.x --> | ||
| <!-- BEGIN: VLT:>=v1.21.x --> |
There was a problem hiding this comment.
Great job adding this testing. ❤️
| } | ||
| } | ||
|
|
||
| const getTfeSemver = (version) => { |
There was a problem hiding this comment.
How come this code is in the vault content exclusion plugin?
There was a problem hiding this comment.
It looks like this might just be poorly named. It seems like you tried to do a remark plugin that handled everything first and then pivoted towards two different plugins to handle these usecases. (I'm indifferent towards either approach.) And this naming is a leftover byproduct of that.
There was a problem hiding this comment.
that function name was an oversight on my part- I used a similar script from mktg-content-workflows as a reference. I would like to refactor all this to a cleaner plugin based OOP approach, but for now I went the route of making the code reuse obvious so that the abstraction can be easier later on
There was a problem hiding this comment.
Considering that Transformer is already a subclass, a subclass of a subclass is usually more trouble then it's worth IMO. Let's just make some shared functions.
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
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