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

Add file modification history update script #121

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

haszi
Copy link
Contributor

@haszi haszi commented Apr 22, 2024

Add file modification history update script. This script can be used to update the fileModHistory.php files in the documentation repos which contain the last modification date/time and the contributor list for every file in the repo.

The ideal way to use this script would be something like a pre-commit hook which would generate the update to the history file, and be merged and potentially reverted together with its parent commit. If anyone knows how to do this on GitHub, please let me know.

@haszi haszi marked this pull request as draft April 23, 2024 08:38
@Girgias
Copy link
Member

Girgias commented May 10, 2024

@shivammathur would you possibly know how to do the git commit hooks?

@shivammathur
Copy link
Member

@Girgias @haszi

To create a pre-commit hook, we have to create a executable .git/hooks/pre-commit file that runs scripts/updateModHistory.php.

But to enforce that this file is there on all clones, we have a couple of options.

If we can add composer to this project, we can run a post install script that copies the pre-commit file to git/hooks/pre-commit.

Another alternative is using https://pre-commit.com/

@haszi
Copy link
Contributor Author

haszi commented May 24, 2024

@shivammathur Thanks for the explanation.

Maybe a pre-commit hook wasn't a good idea for this problem after all. I think there is a way to achieve the same thing in the language repos by the workflow below. I do have some questions though:

Would this workflow trigger for every merge/revert on the repo?
Would the git commit --amend work without any issues?

on:
  push:
    branches:
      - master

jobs:
  if_merged:
    if: github.event.pull_request.merged == true
      runs-on: ubuntu-latest
      steps:
        - name: Checkout documentation
          uses: actions/checkout@v4

        - name: Update fileModHistory.php
          run: |
            # generate the updates to fileModHistory.php
            php updateModHistory.php

            # add and amend generated changes to the last commit
            git add fileModHistory.php            
            git commit --amend

@shivammathur
Copy link
Member

shivammathur commented May 27, 2024

@haszi

It will be better to run it on the pull_request closed event, and then check to make sure it was merged when it was closed.

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-your-pull_request-workflow-when-a-pull-request-merges

I would suggest making a new commit and pushing it to master from CI instead of amending the PR commit. Also to push it to master it will need write permission on the GITHUB_TOKEN secret, so configure that using the permissions key.

on:
  pull_request:
    types:
      - closed
    branches:
      - master
permissions:
  contents: write
jobs:
  if_merged:
    if: github.event.pull_request.merged == true
      runs-on: ubuntu-latest
      steps:
        - name: Checkout documentation
          uses: actions/checkout@v4

        - name: Update fileModHistory.php
          env:
            GITHUB_TOKEN
          run: |
            # generate the updates to fileModHistory.php
            php updateModHistory.php
            
            # Configure git
            git config --local user.email "41898282+github-actions[bot]@users.noreply.github.com"
            git config --local user.name "github-actions[bot]"

            # add and amend generated changes to the last commit
            git add fileModHistory.php            
            git commit -m "Update fileModHistory.php"
        - name: Push changes
          uses: ad-m/github-push-action@master
          with:
            github_token: ${{ secrets.GITHUB_TOKEN }}
            branch: master

@haszi
Copy link
Contributor Author

haszi commented Jun 16, 2024

I've discussed the adding of new commits instead of amending commits with Girgias and the fileModHistory.php file would probably have to be in a dedicated repo so as not to spam the documentation repos' commit history. In that scenario the workflow would still be triggered by the documentation repos after PRs are merged, which would have to pull their respective fileModHistory.php file from its repo, run the update script and commit the changes back to the file's repo.

I do have one question regarding the action/workflow: would this also run the update script when commits are reverted? It would be great if the reverts could use the same workflow even though there probably aren't too many PRs reverted in the doc-* repos.

@shivammathur
Copy link
Member

@haszi Yes, the workflow with pull_request event would run on the revert PR as well.

@haszi haszi force-pushed the Add-file-modification-history-update-script branch from 2a154af to ee8e71c Compare September 1, 2024 12:22
@haszi
Copy link
Contributor Author

haszi commented Sep 1, 2024

@shivammathur I've tested the workflow and it works like a charm. I had to add

      with:
        fetch-depth: 0

under the "Checkout documentation" step to make my script work (it needs the entire git history) and the file modification info file is now being generated perfectly in the same repo.

Could you also point me in the right direction on how to check out the fileModHistory.php file from another repo, run the script to make updates and push the file back to the repo it was checked out of?

@haszi haszi marked this pull request as ready for review October 1, 2024 08:41
@haszi
Copy link
Contributor Author

haszi commented Oct 1, 2024

@Girgias I opened this PR up for review as the script itself is ready. The changes in the last commit make the script accept both the path of the documentation and that of the "history file" as command line parameters, so this can be called (directly or from a GitHub workflow) from within the same or another repo.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I haven't tried it locally yet, but some minor nits.

Also @cmb69 @jimwins may want to have a look

scripts/updateModHistory.php Outdated Show resolved Hide resolved
}
echo "documentation path supplied\n";
if (isset($commandLineOptions["history-path"])) {
echo " mod history file path supplied\n";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe have a variable for the space indent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used strlen() in my last commit. Will that work too or would a variable be better in this case?

@haszi haszi requested review from cmb69 and jimwins October 2, 2024 21:35
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Sorry, I do not understand the purpose of having a fileModHistory.php file.

@haszi
Copy link
Contributor Author

haszi commented Oct 4, 2024

Sorry, I do not understand the purpose of having a fileModHistory.php file.

My bad!

The idea behind this file and the script is that every time a PR is merged into any of the documentations, the script generates a file (fileModHistory.php) with a PHP associative array. This array contains the hash of the last merged commit and a the list of all files in that doc repo. For each file, the last date/time that file was updated and list of all commit authors is recorded as well. This information then would be shown on each of the documentation pages (see this PR for screenshots) and could be used to generate a list of all authors that could be added to the list of authors/editors on the Preface page.

The script in this PR is for generating the file (fileModHistory.php) with the all the above information and it should work from either the command line or from a GitHub workflow. The script would be used like this:

php updateModHistory.php --docs-path=/path/doc/doc-repo --history-path=/path/to/existing/fileModHistoryFile.php

@cmb69
Copy link
Member

cmb69 commented Oct 5, 2024

Thanks @haszi for the explanation! While this is low priority for me, I understand that it will be more important for others, so I'm generally fine with the approach.

scripts/updateModHistory.php Outdated Show resolved Hide resolved
@haszi haszi requested review from cmb69 and Girgias October 14, 2024 06:31
Comment on lines +116 to +118
$newModHistoryString = ' "' . $fileName . "\" => [\n";
$newModHistoryString .= " \"modified\" => \"" . ($fileProps["modified"] ?? "") . "\",\n";
$newModHistoryString .= " \"contributors\" => [\n";
Copy link
Member

Choose a reason for hiding this comment

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

Use sprintf or var_export here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like this?

$newModHistoryString = sprintf("%4s\"%s\" => [\n%8s\"modified\" => \"%s\",\n%8s\"contributors\" => [\n", 
    " ", 
    $fileName, 
    " ", 
    $fileProps["modified"] ?? "",
    " "
);

Copy link
Member

Choose a reason for hiding this comment

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

Possibly, but this really minor actually.

Comment on lines +127 to +128
$newModHistoryString .= " ],\n";
$newModHistoryString .= " ],\n";
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this work?

$newModHistoryString .= sprintf("%8s],\n%4s],\n", " ", " ");

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

I guess this is good; can't test anyway, since I cannot afford a bash. :)

@Girgias Girgias merged commit 7622734 into php:master Oct 21, 2024
10 of 12 checks passed
@haszi haszi deleted the Add-file-modification-history-update-script branch October 21, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants