ci(check-ci-skip): fix commitMessagesMetadata.forEach is not a function #3618
Conversation
zondervancalvez
requested review from
petermetz,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
zondervancalvez
force-pushed
the
zondervancalvez/issue3614
branch
from
abbd1f9
to
0c47b08
Compare
There was a problem hiding this comment.
@zondervancalvez I don't think you have checked the code difference when creating this PR. Its totally wrong as you have already commented all the input arguments and hard-coded them with constant fields.
Also the check you have added should output a result which would be self explanatory. Saying Commit message data is empty or null doesn't help the creator of a PR. The right message should be Fetching data from github API failed. This is most likely due to an intermittent issue from Github end or the api being rate limited (one hour cooldown). Please retry running the failed check after an hour.
Also, it would be much better if we fine tune the error handling so that we can clearly say if it is an intermittent issue or rate limit issue.
Although rate limit errors are less likely to occur as the limit for the hosted runners is 1000 per hour
Also, even after mentioning it multiple times, yet we are at the same point where I have to specifically mention that ## Commit and ## **Commit** are rendered same.
zondervancalvez
force-pushed
the
zondervancalvez/issue3614
branch
3 times, most recently
from
1398b2c
to
1178be7
Compare
There was a problem hiding this comment.
@zondervancalvez Is this diff going to also fix the issue that if the check script crashes, the CI jobs get skipped by default?
tools/ci-skip-for-maintainers.js
Outdated
| commitMessageList.push(commitMessageMetadata["commit"]["message"]); | ||
| }); | ||
| } else { | ||
| console.error( |
There was a problem hiding this comment.
@zondervancalvez Instead of just logging on stderr we should crash. Otherwise there will be more unintended side-effects to the error.
The bottom line is that if we canot fetch the commit message metadata, then we cannot complete the check at all.
@zondervancalvez Is it possible to refactor this code to use the local git installation and list the commits that way so that we are not hitting the GitHub API at all for this?
zondervancalvez
force-pushed
the
zondervancalvez/issue3614
branch
2 times, most recently
from
8ca65ca
to
38a7d3f
Compare
There was a problem hiding this comment.
@zondervancalvez Do these changes solve the problem that if the ci-ckip-for-maintainers.js code crashes, it implicitly skips all the test checks?
tools/ci-skip-for-maintainers.js
Outdated
| console.log("Latest commit message:\n", commitMessage); | ||
| } catch (error) { | ||
| console.error("Error fetching commit message:\n", error.message); | ||
| } | ||
|
|
||
| // Check if skip-ci is found in commit message | ||
| const checkSkipCI = () => { |
There was a problem hiding this comment.
@zondervancalvez Please have all the functions be non-nested functions.
zondervancalvez
force-pushed
the
zondervancalvez/issue3614
branch
3 times, most recently
from
3227fee
to
1e0129b
Compare
There was a problem hiding this comment.
@zondervancalvez Could you please address the questions above?
|
Primary Changes ---------------- 1. Changed the method in getting the commit message from GitHub API to shell command to avoid the rate limits in calling the API. 2. Same goes for the author of commit message, we use shell command to fetch the username. Fixes hyperledger-cacti#3614 Signed-off-by: bado <[email protected]>
zondervancalvez
force-pushed
the
zondervancalvez/issue3614
branch
from
1e0129b
to
7289fe9
Compare
@zondervancalvez I'm sorry, I can't find the answer to my question from earlier regarding the crashes. Could you please be more specific about which part of the commit message is answering that question? |
| //const committerLogin = "<username>"; | ||
| // Function to get the latest commit message | ||
| const getLatestCommitMessage = () => { | ||
| const commitMsgBuffer = execSync("git log -1 --pretty=%B"); |
There was a problem hiding this comment.
@petermetz to answer your question, this git log shell command is the alternative we used for fetching commit messages instead of calling github API to avoid limits and crashing
Commit to be reviewed
ci(check-ci-skip): fix commitMessagesMetadata.forEach is not a function
Fixes #3614
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.