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

Throw when values are updated in CI #17

Closed
wants to merge 4 commits into from
Closed

Conversation

10hendersonm
Copy link
Member

@10hendersonm 10hendersonm commented Aug 16, 2021

closes #6

⚠️ Breaking Change

Features

CI-Aware

markdown-inject is now CI-aware! It will exits with code 1 without writing new blocks when CI=true and block changes exist. This behavior can be bypassed with the --force-write (-f) flag. I've added a CI step to perform this validation internally.

Prepare: build

The build npm script now occurs on prepare for lazier local development. This is / can be bypassed with CI=true, meaning a CI build step that is separate from the install step is still necessary. This is intentional to ease isolating CI-time build failures.

Validation

Automated

I've added a Check Markdown Docs step to CI, which you can observe (intentionally) failing here:

https://github.com/target/markdown-inject/runs/3906606163

Also, note that no build occurs during the Install Dependencies step.

Manual

Due to the addition to the prepare script, installing dependencies will now cause a build to occur:

npm ci
Result

image

Alternatively, setting CI=true will prevent this build:

CI=true npm ci
Result

image

Let's write a junk markdown block to validate some things:

echo '<!-- CODEBLOCK_START {"type": "command", "value": "echo It is currently $(date)"} -->\n<!-- CODEBLOCK_END -->' > pr-17-test.md
Result

image

Let's make sure we can still write markdown normally:

npm run markdown-inject
Result

image

Now, let's re-write the empty block and try to write it with CI=true. The failure is expected, and the error text indicates that (1) block changes do not occur; (2) block changes are prohibited in CI; and (3) this can be bypassed with the --force-write CLI flag.

echo '<!-- CODEBLOCK_START {"type": "command", "value": "echo It is currently $(date)"} -->\n<!-- CODEBLOCK_END -->' > pr-17-test.md

CI=true npm run markdown-inject
Result

image

Let's put that CLI flag to the test:

CI=true npm run markdown-inject -- --force-write

ℹ️ The extra -- before --force-write is because we call markdown-inject inside this repo with an npm script, and need to pass the arguments over npm so they are provided to the script itself.

Result

image

@10hendersonm 10hendersonm marked this pull request as ready for review October 15, 2021 13:37
@10hendersonm 10hendersonm self-assigned this Oct 15, 2021
Copy link
Contributor

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

On my Mac and in a bash environment, this works flawlessly and has great validation steps and testing. Nice work!

Some comments within to discuss.

Here and elsewhere, I suspect we've strayed from official cross-platform support. It breaks in cmd and Powershell. I don't know if we care to make that supported - but the creeping in of these assumptions should be noted in my opinion.

@@ -23,3 +23,6 @@ jobs:

- name: Build
run: npm run build

- name: Check Markdown Docs
run: npm run markdown-inject
Copy link
Contributor

Choose a reason for hiding this comment

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

love this is an integration test - it looks to small to be useful but really it is!

README.md Show resolved Hide resolved
@@ -16,7 +16,7 @@
"clean": "rm -rf dist",
"lint": "eslint .",
"test": "jest",
"prepare": "husky install",
"prepare": "husky install && ( [ \"$CI\" = 'true' ] || npm run build )",
Copy link
Contributor

Choose a reason for hiding this comment

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

This and clean are not directly Windows friendly

image

Of course a bash or zsh prompt from within VS Code works.

src/__tests__/md-inject.test.ts Show resolved Hide resolved
@bmuenzenmeyer
Copy link
Contributor

closing this - it's really stale and we've changed a few things since

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail when blocks are written and CI=true
2 participants