-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
chore: updates to changesets workflow #14559
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
Conversation
|
- IMPORTANT: Ensure there is a newline before and after the code blocks in your feedback. Having triple backticks on the same line as other text WILL cause rendering issues. | ||
- ALWAYS check your feedback for proper markdown formatting, particularly code fences and backticks. | ||
- Use markdown codeblocks with triple backticks: 'yaml' for YAML examples, 'markdown' for changeset body examples | ||
- IMPORTANT: Ensure there is a newline before and after code blocks. Triple backticks on the same line as other text WILL cause rendering issues |
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.
Just noting that it's not uncommon for us to have triple backticks and diff
on the same line! (Niche case, but not sure whether worth mentioning here)
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.
Approving for the updated validation rules! (Noted one possible exception in a comment)
✓ Correct: "Fixes", "Adds", "Updates", "Removes" | ||
✗ Incorrect: "Fix", "Add", "Update", "Remove", "Fixed", "Added" |
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.
Do symbols really help here? Geniune question
```markdown | ||
--- | ||
'astro': minor | ||
--- | ||
|
||
Adds support for custom error pages in middleware. You can now return a custom error response: | ||
|
||
\`\`\`js | ||
export function onRequest(context, next) { | ||
if (!context.locals.user) { | ||
return new Response('Unauthorized', { status: 401 }); | ||
} | ||
return next(); | ||
} | ||
\`\`\` | ||
``` | ||
|
||
```markdown |
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.
Very hard to provide a suggestion with markdown and UI, but usually here you can create "nested" code blocks:
``````md
## Heading
```js
const foo = "bar";
```
```````
- name: Set up Node.js | ||
uses: actions/setup-node@2028fbc5c25fe9cf00d9f06a71cc4710d4507903 # v6.0.0 | ||
with: | ||
node-version: '20' |
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.
node-version: '20' | |
node-version: 'lts/*' |
So we don't need to update it
# Check for shallow headings in each changeset | ||
echo "::debug::Checking for shallow headings (h1, h2, h3) in changesets" | ||
while IFS= read -r file; do | ||
if [ -f "$file" ]; then | ||
# Make path absolute for the script | ||
abs_file="$(pwd)/$file" | ||
# Run the script and capture output | ||
if heading_output=$(pnpm --filter astro-scripts has-shallow-headings "$abs_file" 2>&1); then | ||
echo "::debug::$file: No shallow headings found" | ||
else | ||
# Filter output to only lines that look like markdown headings (start with #) | ||
# This removes pnpm wrapper noise | ||
heading_output=$(pnpm --filter astro-scripts has-shallow-headings "$abs_file" 2>&1 | grep '^#' || true) | ||
if [ -n "$heading_output" ]; then | ||
echo "::warning::$file: Contains forbidden shallow headings" | ||
# Add heading errors to the validation response | ||
# The script outputs the offending headings, one per line | ||
while IFS= read -r heading; do | ||
if [ -n "$heading" ]; then | ||
error_msg="Forbidden heading found: '$heading'. Headings shallower than h4 (####) break changelog formatting. Please use h4 or deeper." | ||
echo "::debug::Adding heading error for $file: $heading" | ||
# Add error to the file's general_errors array in the JSON | ||
jq --arg file "$file" --arg error "$error_msg" ' | ||
.overall_valid = false | | ||
.files |= map( | ||
if .file == $file then | ||
.valid = false | | ||
.general_errors += [$error] | ||
else . end | ||
) | ||
' validation_response.json > validation_response.tmp.json | ||
mv validation_response.tmp.json validation_response.json | ||
fi | ||
done <<< "$heading_output" | ||
else | ||
echo "::debug::$file: Script failed but no headings found (might be other error)" | ||
fi | ||
fi | ||
fi | ||
done < changeset_files.txt |
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 wonder if it's best to have a JS script instead. I doubt any of us will be able to maintain it
Changes
Testing
Tested on a branch using
pull_request
, before switching topull_request_target
for this PR. As ever withpull_request_target
PRs, this part will only be tested when it's actually merged and a new PR opened. Here's the PR with test changesets and comments: ascorbic#2Example of the output:
Docs