Skip to content

Conversation

@lusu007
Copy link
Member

@lusu007 lusu007 commented Jul 18, 2024

Fixes #32
Fixes #27

Additional Context

N/A

Checklist

  • The chart version in Chart.yaml has been updated according to semantic versioning (semver).
  • All variables are documented in the Chart's values.yaml and README.md files.
  • The pull request title meets the Conventional Commits specification and includes the chart name, for example: feat(chart-name): Add replica support

@lusu007 lusu007 self-assigned this Jul 18, 2024
@lusu007
Copy link
Member Author

lusu007 commented Jul 18, 2024

  • Ensure only one Chart is updated in a PR
  • Generate commit messages dynamically, reflecting only the modified files.
  • Streamline the process of updating README.md, focusing solely on changes in a single Chart.
  • Split into multiple Jobs

@meyfa
Copy link
Member

meyfa commented Jul 18, 2024

Generate commit messages dynamically, reflecting only the modified files

I doubt this is needed. It can just be "docs(chart): Generate documentation" for example. No need to overcomplicate.

Co-authored-by: Fabian Meyer <3982806+meyfa@users.noreply.github.com>
Copy link
Member

@meyfa meyfa left a comment

Choose a reason for hiding this comment

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

The workflow is currently vulnerable to at least one practical attack. Please make sure that all user input is quoted and validated before being used in a shell.

@lusu007 lusu007 marked this pull request as ready for review July 22, 2024 19:56
@lusu007 lusu007 requested a review from a team as a code owner July 22, 2024 19:56
Comment on lines +109 to +110
changes="$changes"$'\n' - kind: feature
changes="$changes"$'\n' description: "${PR_TITLE#*: }"
Copy link
Member

Choose a reason for hiding this comment

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

I've never seen this syntax before... What does the second $ do, and why are there no quotes surrounding - kind: feature etc?

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, if there was a quote in the PR title, it would need to be escaped. We might have to use a tool for that, since any backslash in the PR title would also need to be escaped, etc. In general, this can often lead to invalid YAML. Not a security risk, just something that could lead to broken commits.

Comment on lines +109 to +110
changes="$changes"$'\n' - kind: feature
changes="$changes"$'\n' description: "${PR_TITLE#*: }"
Copy link
Member

Choose a reason for hiding this comment

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

Additionally, if there was a quote in the PR title, it would need to be escaped. We might have to use a tool for that, since any backslash in the PR title would also need to be escaped, etc. In general, this can often lead to invalid YAML. Not a security risk, just something that could lead to broken commits.

Comment on lines +106 to +112
if [[ $PR_TITLE == *"feat:"* ]]; then
changes="$changes"$'\n' - kind: feature
changes="$changes"$'\n' description: "${PR_TITLE#*: }"
elif [[ "$PR_TITLE" == *"fix:"* ]]; then
changes="$changes"$'\n' - kind: bugfix
changes="$changes"$'\n' description: "${PR_TITLE#*: }"
fi
Copy link
Member

@meyfa meyfa Jul 23, 2024

Choose a reason for hiding this comment

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

Wouldn't this work? (Besides generating invalid YAML if the PR title contains quotes or backslashes)

Suggested change
if [[ $PR_TITLE == *"feat:"* ]]; then
changes="$changes"$'\n' - kind: feature
changes="$changes"$'\n' description: "${PR_TITLE#*: }"
elif [[ "$PR_TITLE" == *"fix:"* ]]; then
changes="$changes"$'\n' - kind: bugfix
changes="$changes"$'\n' description: "${PR_TITLE#*: }"
fi
if [[ "$PR_TITLE" == "feat"* ]]; then
changes=" - kind: feature\n description: \"${PR_TITLE#*: }\""
elif [[ "$PR_TITLE" == "fix"* ]]; then
changes=" - kind: bugfix\n description: \"${PR_TITLE#*: }\""
fi

Comment on lines +119 to +128
# Check if annotations exist
if ! grep -q "annotations:" <<< "$chart"; then
chart=$(echo "$chart"$'\n'annotations:)
fi
# Check if artifacthub.io/changes annotation exists
if ! grep -q "artifacthub.io/changes:" <<< "$chart"; then
chart=$(echo "$chart"$'\n' artifacthub.io/changes: | sed 's/^/ /')
fi
# Append the new changes to the existing ones
chart=$(echo "$chart"$'\n'"$changes" | sed 's/^/ /')
Copy link
Member

Choose a reason for hiding this comment

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

I'm really not sure what this part is trying to do 😅 Why would we always append? Shouldn't we replace the annotations from the previous release? In any case, I don't think the current syntax achieves this - however, I'm struggling right now to come up with a good solution that is guaranteed not to break the YAML.

Maybe Bash isn't the right tool for the job, after all.

Comment on lines +92 to +96
echo "Generating changelog for $CHANGED_CHART"
npx semantic-release --dry-run --no-ci --plugins @semantic-release/release-notes-generator > "$CHANGED_CHART/release-notes.md"
# Extract relevant part
sed -n '/### \[/{:a;n;/### \[/{p;q};p;ba}' "$CHANGED_CHART/release-notes.md" > "$CHANGED_CHART/CHANGELOG.md"
rm "$CHANGED_CHART/release-notes.md"
Copy link
Member

Choose a reason for hiding this comment

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

This will get rid of any previous content in CHANGELOG.md, right? Usually, we would want the changelog to contain all versions ever released. This is also important e.g. if Renovate jumps over a version (such as updating from 1.2.0 straight to 1.4.0, in which case it should also have access to the 1.3.0 changelog).

- name: Commit and push changes
run: |
git add .
git commit -m "docs(${CHANGED_CHART%*/}): Generate documentation" # Remove trailing slash
Copy link
Member

Choose a reason for hiding this comment

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

The previous workflow would not create a commit if the README.md was up-to-date. This workflow will create a commit on every run, even if README.md and CHANGELOG.md are unchanged. In the best case, this will fail since --allow-empty is not passed to git commit, but it may also trigger the dreaded infinite loop in CI.

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.

Add Artifacthub changelog annotations Add a CHANGELOG.md for each chart

3 participants