-
Notifications
You must be signed in to change notification settings - Fork 2.7k
ci: add script to check PRs against CHANGELOG.md for completeness #12309
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
) | ||
|
||
# Configure PR numbers to ignore | ||
IGNORE_PRS=( |
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.
refer: #12308 (comment)
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.
Please use a TypeScript script instead of bash. This is not at all easy to review and maintain.
This has nothing to do with the APISIX runtime and does not have any negative performance impact, so it should definitely be developed in the most efficient language.
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 don’t quite agree. From a project maintenance perspective, I believe we should minimize the number of languages used. If you prefer TypeScript and find it more efficient, then by the same logic, others might argue for using Python if they find it more efficient—should we adopt Python too?
Although I’ve written TypeScript for a long time, I still chose Bash for this script because, regardless of the language used, the core task involves extracting PR numbers via git commands and regex parsing of the changelog. The difference in implementation efficiency isn’t significant. Using TypeScript would also require setting up a Node.js environment, which might be unfamiliar to some contributors, and won’t necessarily result in a noticeable efficiency gain.
Regarding review and maintenance: I’ve added detailed comments in the script. The shell code mainly consists of commonly used commands. With the help of AI tools, I don’t think it’s particularly hard to understand or maintain.
Waiting for others options. @nic-6443 @AlinsRan @membphis @moonming
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.
From a project maintenance perspective
If one must look at it from a maintenance point of view, there are more problems with this script.
Why are logic and data coupled in a single script file? This forces every developer who wishes to add a special case to understand the script. Why not use a separate JSON file to store these exceptions?
I believe we should minimize the number of languages used
Reducing the number of languages is not the goal, but rather using the right languages in the right places, languages that really have a wide range of users and can be used easily.
Another fact is that TypeScript has already been adopted by the project and was first applied to the testing part.
From some of the feedback I've heard, it's more popular than test-nginx.
Python has not achieved this status.
If you prefer TypeScript and find it more efficient
First of all, I admit that the choice of TypeScript is my personal preference. But so what?
There is no doubt that the JavaScript/TypeScript runtime based on v8 or any other JIT-enabled compiler is faster than Python.
CPython has no production-ready built-in JIT implementation to date, and PyPy is not widely used.
I don't see how these two can gain superiority over each other in terms of development efficiency. But, in any case, they are both better than the shell.
regardless of the language used, the core task involves extracting PR numbers via git commands and regex parsing of the changelog
This is the point of using a “modern” tool, where we can express clearer logic with less code. And we will have countless libraries to use to reduce our own workload.
Compactness is now less of an advantage and more of a disadvantage, which brings low readability.
Using TypeScript would also require setting up a Node.js environment, which might be unfamiliar to some contributors
This proves once again the significance of splitting logic and data. Unless we need to deal with a new situation, it's straightforward for a developer to add a CHANGELOG special case by simply updating a copy of the JSON/YAML configuration.
The CI should definitely give developers a clear indication that you need to add a CHANGELOG or how to add a special case on demand.
Now the developer will have to edit your script documentation, which can introduce bugs in an improper review.
and won’t necessarily result in a noticeable efficiency gain
I do not agree. I'm not a git expert, and if I didn't consult the documentation, I probably wouldn't be able to say what each of the git commands included in the script you generated with the help of AI does and what the arguments in it represent.
When we implement it semantically, it will be simpler.
https://github.com/apache/apisix-website/blob/master/scripts/sync-docs.js#L41-L53
https://www.npmjs.com/package/simple-git
On top of that, you're using awk to work with strings. Can you guarantee to understand and reproduce this without the help of AI? How long would it take you to figure out all these git and awk commands?
With all due respect, this is hardly readable.
With the help of AI tools, I don’t think it’s particularly hard to understand or maintain.
If a piece of code is important enough to require manual review, then I don't trust any snippet generated directly by AI. They don't even solve the problem of removing the illusion.
Can the author fully understand what it's generating? Do those comment feet really clearly explain every command used?
For example, that awk command: https://github.com/apache/apisix/pull/12309/files#diff-67d26cf11e0379ef7aa36cf327b9154b737a0e4dd38905b74c8bec9cb6c31789R90-R97
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.
-
Developers can leverage AI with various tools, but we must fully understand the code generated by the AI and not be "controlled" by it.
-
From a project maintenance perspective, code that is easier to understand significantly improves maintenance, even if the language has not been used before. Currently, it is hard to understand what this parts (https://github.com/apache/apisix/pull/12309/files#diff-67d26cf11e0379ef7aa36cf327b9154b737a0e4dd38905b74c8bec9cb6c31789R90-R97).
-
Regarding the need to configure the environment for CI work, this is not difficult to address and maintain. Using the right tool in right place is right.
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.
Alright, since you all think TypeScript is more appropriate, I’ll try to submit a new PR using TypeScript when I have time. But I still stand by my opinion: using a Bash script is more concise, though it sacrifices some readability.
Also, if I didn’t understand the code, I wouldn’t submit a PR for review — that’s being responsible to the community reviewer and to myself.
Anyway, thanks for the review. I will submit a new PR to complete it using TypeScript.
Description
This PR provides a script to check the changelog for completeness. If a PR is not of type docs, chore, test, or ci, and is not listed in the IGNORE_PRS variable, but is missing from the changelog, the script will raise an error.
Note: This script currently only checks changelog entries for versions after 3.8.0.
Test record:
Which issue(s) this PR fixes:
Fixes #
Checklist