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

adding the script to validate changelog #4731

Merged
merged 14 commits into from
Aug 17, 2023

Conversation

nitishchauhan0022
Copy link
Contributor

@nitishchauhan0022 nitishchauhan0022 commented Jun 23, 2023

adding the script to validate changelog by make validate-changelog and updating github action

Checklist

Fixes #3190

@nitishchauhan0022 nitishchauhan0022 requested a review from a team as a code owner June 23, 2023 06:44
@github-actions
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Thanks for this! The changelog is sorted, but we also tend to put **General**: before all other values.
Also would be nice to add a way to skip the Changelog validation for a certain PR. (eg. we have the skip-e2e label for skipping e2e test check).

@JorTurFer
Copy link
Member

Also would be nice to add a way to skip the Changelog validation for a certain PR. (eg. we have the skip-e2e label for skipping e2e test check)

I'm not sure if we should be able to skip Changelog validation, it's more like other CI checks than e2e test IMO, we cannot skip static checks (and this is one static check more). Personally, I'm fine without skipping it, but I prefer to execute it as part of pre-commit if it's possible. If somebody is using pre-commit, it'll cover this new check automatically

WDYT?

@zroubalik
Copy link
Member

Also would be nice to add a way to skip the Changelog validation for a certain PR. (eg. we have the skip-e2e label for skipping e2e test check)

I'm not sure if we should be able to skip Changelog validation, it's more like other CI checks than e2e test IMO, we cannot skip static checks (and this is one static check more). Personally, I'm fine without skipping it, but I prefer to execute it as part of pre-commit if it's possible. If somebody is using pre-commit, it'll cover this new check automatically

WDYT?

Good point!

@nitishchauhan0022
Copy link
Contributor Author

nitishchauhan0022 commented Aug 11, 2023

hey @JorTurFer @zroubalik Please take a look.

Signed-off-by: ntishchauhan0022 <nitishchauhan0022@gmail.com>
@nitishchauhan0022
Copy link
Contributor Author

nitishchauhan0022 commented Aug 11, 2023

Thanks for this! The changelog is sorted, but we also tend to put General: before all other values.

@zroubalik To my understanding as there are other values as well checking for General specifically will not give us any result. As we than also need to allow for other **example name** also. If we can limit those names than that check would be a good fit. WDYT?

Signed-off-by: ntishchauhan0022 <nitishchauhan0022@gmail.com>
@JorTurFer
Copy link
Member

Thanks for this! The changelog is sorted, but we also tend to put General: before all other values.

@zroubalik To my understanding as there are other values as well checking for General specifically will not give us any result. As we than also need to allow for other example name also. If we can limit those names than that check would be a good fit. WDYT?

General is the only value that has to be prioritized. This is because usually General affects to all the users and that's why we place it on top of all the sections. The style rules are here: https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#Changelog

Said that, maybe we can add 2 steps in the extract_and_check function, the first step checks for General items and the second one for the rest. WDYT?

@nitishchauhan0022
Copy link
Contributor Author

@JorTurFer sounds great, i will add it.

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

This is nice! My only question is if we can check all the changelog and not just unreleased.
I say this because during cherry-picks for hotfix releases we can make mistakes

@nitishchauhan0022
Copy link
Contributor Author

nitishchauhan0022 commented Aug 15, 2023

This is nice! My only question is if we can check all the changelog and not just unreleased. I say this because during cherry-picks for hotfix releases we can make mistakes

we can but we need to change all previous to correct order as well first. Some also have entries missing as well. Like having change name only and doesn't having pr reference.

@JorTurFer
Copy link
Member

we can but we need to change all previous to correct order as well first.

If we can, it'd be nice. The changelog has been chaotic in the past 😞

@nitishchauhan0022
Copy link
Contributor Author

we can but we need to change all previous to correct order as well first.

If we can, it'd be nice. The changelog has been chaotic in the past disappointed

I will try.

Signed-off-by: ntishchauhan0022 <nitishchauhan0022@gmail.com>
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

This looks nice!
Let me check it with some cases locally and that'd be all I forgot the point of all the versions
Please, remember to undo the change in pr-validation.yaml

CHANGELOG.md Outdated Show resolved Hide resolved
@nitishchauhan0022
Copy link
Contributor Author

hey @JorTurFer I have tried the script to validate the change in each version. The script is:

#!/bin/bash

SCRIPT_ROOT=$(dirname "${BASH_SOURCE[0]}")/..

# Define filename
filename="$SCRIPT_ROOT/CHANGELOG.md"

# Check if file exists
if [[ ! -f "$filename" ]]; then
    echo "Error: $filename does not exist."
    exit 1
fi

# Storing the version to be checked
mapfile -t versions < <(awk '/## History/{flag=1;next}/## /{flag=0}flag' "$filename" | grep -o '\[[^]]*\]' | sed 's/[][]//g')

# Define a function to extract and sort sections
function extract_and_check() {
  local section=$1
  local content_block=$2
  local content=$(awk "/### $section/{flag=1;next}/### /{flag=0}flag" <<< "$content_block" | grep '^- \*\*')

  # Skip if content does not exist
  if [[ -z "$content" ]]; then
    return
  fi

  # Separate and sort the **General**: lines
  local sorted_general_lines=$(echo "$content" | grep '^- \*\*General\*\*:' | sort)

  # Sort the remaining lines
  local sorted_content=$(echo "$content" | grep -v '^- \*\*General\*\*:' | sort)

  # Check if sorted_general_lines is not empty, then concatenate
  if [[ -n "$sorted_general_lines" ]]; then
      sorted_content=$(printf "%s\n%s" "$sorted_general_lines" "$sorted_content")
  fi

  # Check pattern and throw error if wrong pattern found
  while IFS= read -r line; do
      echo "Error: Wrong pattern found in section: $section , line: $line"
      exit 1
  done < <(grep -Pv '^(-\s\*\*[^*]+\*\*: .*\(\[#(\d+)\]\(https:\/\/github\.com\/kedacore\/(keda|charts)\/(pull|issues)\/\2\)\))$' <<< "$content")

  if [ "$content" != "$sorted_content" ]; then
      echo "Error: Section: $section is not sorted correctly. Correct order:"
      echo "$sorted_content"
      exit 1
  fi
}


# Extract release sections, including "Unreleased", and check them
for version in "${versions[@]}"; do
  release_content=$(awk "/## $version/{flag=1;next}/## v[0-9\.]+/{flag=0}flag" "$filename")


  if [[ -z "$release_content" ]]; then
    echo "No content found for $version Skipping."
    continue
  fi

  echo "Checking section: $version"

  # Separate content into different sections and check sorting for each release
  extract_and_check "New" "$release_content"
  extract_and_check "Improvements" "$release_content"
  extract_and_check "Fixes" "$release_content"
  extract_and_check "Deprecations" "$release_content"
  extract_and_check "Other" "$release_content"

done

But the problem is, some changes are not logged properly, and not easy to search for in the pr and issues, may be they are named differently there.
For example:

Checking section: v2.11.0
Error: Wrong pattern found in section: Improvements , line: - **General**: Enable secret scanning in GitHub repo

Signed-off-by: ntishchauhan0022 <nitishchauhan0022@gmail.com>
@JorTurFer
Copy link
Member

But the problem is, some changes are not logged properly, and not easy to search for in the pr and issues, may be they are named differently there.

Let me take a look, we haven't been following the same rules since the beginning, let me check it changelog for trying to limit the scope 😄

Signed-off-by: ntishchauhan0022 <nitishchauhan0022@gmail.com>
@JorTurFer
Copy link
Member

There are a lot of issues on the changelog T.T
Can I fix them as part of your PR? As you enabled maintainer to edit the PR, I can commit into your branch if you agree

@JorTurFer
Copy link
Member

Based on your script, I have done some small modifications (to exclude v1.* and to support multiple repetitions of the pattern) and I have fixed all the issues on the changelog. After that, it works really nice ❤️

As I said, if you agree, I can commit the changes to your branch directly

@JorTurFer
Copy link
Member

image

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@nitishchauhan0022
Copy link
Contributor Author

@JorTurFer yes please. Thankyou

@JorTurFer
Copy link
Member

Done!
Please take a look 🙏 I'm terrible using bash and probably I have done any mistake. I started from your code and update it a bit, so I hope that I didn't do too much shity code xD

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member

/skip-e2e

JorTurFer and others added 2 commits August 17, 2023 09:26
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@nitishchauhan0022
Copy link
Contributor Author

Done! Please take a look 🙏 I'm terrible using bash and probably I have done any mistake. I started from your code and update it a bit, so I hope that I didn't do too much shity code xD

Thanks for your kind words, It looks good to me.

@nitishchauhan0022
Copy link
Contributor Author

Why is failing in this:
d6c54d5
Same is running fine locally.

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member

JorTurFer commented Aug 17, 2023

Why is failing in this:
d6c54d5
Same is running fine locally.

That failure that the commit solves is related with missing new line at the end of the file. It's this rule: https://github.com/kedacore/keda/blob/main/.pre-commit-config.yaml#L16C7-L16

About why the script that validates the changelog failed, it looks related with some small differences between grep and sort commands in Windows, Linux and Mac. For example, the grep modifier -P doesn't exist in Mac (I have replaced it with -E) and sort has different default behaviours (in Windows using WSL, default behaviour is case insensitive but in Mac is case sensitive...)

My latest commit updates some lines to be explicit: 2739bb1

Let's see 🤞

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member

Everything is green! 🎉 🎉 🎉 🎉

image

@JorTurFer
Copy link
Member

@zroubalik PTAL

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@tomkerkhove tomkerkhove merged commit 8fa1139 into kedacore:main Aug 17, 2023
yoongon pushed a commit to yoongon/keda that referenced this pull request Aug 26, 2023
Co-authored-by: Jorge Turrado <jorge_turrado@hotmail.es>
Co-authored-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Yoon Park <yoongon.park@gmail.com>
Adarsh-verma-14 pushed a commit to Adarsh-verma-14/keda that referenced this pull request Sep 4, 2023
Co-authored-by: Jorge Turrado <jorge_turrado@hotmail.es>
Co-authored-by: Jorge Turrado <jorge.turrado@scrm.lidl>
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
Co-authored-by: Jorge Turrado <jorge_turrado@hotmail.es>
Co-authored-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a check to validate the changelog on PRs
5 participants