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

Updated Daniel Ridge workflows #23

Merged
merged 15 commits into from
Aug 14, 2024
Merged

Conversation

andyphancode
Copy link
Contributor

@andyphancode andyphancode commented Jun 17, 2024

Fixes #6979

What changes did you make?

  • Modified actions to run on Node.js 20 instead of Node.js 16 by updating checkout actions/checkout@v3 to v4 (see Check out Daniel Ridge repo step on both workflow files).
  • Changed Check for modified files step result to be set in Environment variable instead of variable through to-be deprecated set-output method (see Check for modified files step on both workflows).
  • Changed commit and issue creation parts of both workflows to check for env.modified instead of set-output variable, steps.git-check.outputs.modified (see last two steps on both workflow files).

Why did you make the changes (we will use this info to test?

  • Automated workflows in Daniel Ridge repository were showing warnings of set-output and Node.js 16 actions being deprecated, so adjustments were made to update to address those warnings.

Below are screenshots of manually triggered workflows showing no warnings as compared to before.

Workflow logs

Before:

gh pages workflow before 1

After:

gh pages workflow after 1

Workflow jobs page

Before

gh pages workflow warnings before

After

gh pages workflow warnings after

@andyphancode andyphancode marked this pull request as draft June 17, 2024 08:04
@andyphancode andyphancode marked this pull request as ready for review June 17, 2024 08:06
@andyphancode
Copy link
Contributor Author

Wanted to note two things:

Test by running .github/workflows/check-gh-pages-version.yml and verify that the appropriate issues were created

Cannot complete this step of original issue due to current versions being up to date on both workflows. No issues are made as versions are up to date, but workflow passes as expected. I assumed there should be no issue since the issue creation steps of the workflows were not modified at all nor mentioned to have any problems. Recent workflows on the original repository have also passed with no issues.

Originally tried to replace the set-output method as such:

From:
- name: Check for modified files id: git-check run: echo ::set-output name=modified::$([ -z "git status --porcelain" ] && echo "false" || echo "true")
To:
- name: Check for modified files id: git-check run: echo "modified=$([ -z \"$(git status --porcelain)\" ] && echo false || echo true)" >> $GITHUB_ENV

but modified variable would continue to read as true despite lack of modified files. As a result, I changed it to the explicit if/else conditional that is currently in this PR but let me know if that is not satisfactory.

@andyphancode
Copy link
Contributor Author

Wanted to note two things:

Test by running .github/workflows/check-gh-pages-version.yml and verify that the appropriate issues were created

Cannot complete this step of original issue due to current versions being up to date on both workflows. No issues are made as versions are up to date, but workflow passes as expected. I assumed there should be no issue since the issue creation steps of the workflows were not modified at all nor mentioned to have any problems. Recent workflows on the original repository have also passed with no issues. Let me know if this needs to be explicitly tested or if it's fine not to, I am not exactly sure how to prompt the workflow to create an issue when the gh-pages and ruby version are up to date.

Originally tried to replace the set-output method as such:

From:
- name: Check for modified files id: git-check run: echo ::set-output name=modified::$([ -z "git status --porcelain" ] && echo "false" || echo "true")
To:
- name: Check for modified files id: git-check run: echo "modified=$([ -z \"$(git status --porcelain)\" ] && echo false || echo true)" >> $GITHUB_ENV

but modified variable would continue to read as true despite lack of modified files. As a result, I changed it to the explicit if/else conditional that is currently in this PR but let me know if that is not satisfactory.

@danielridgebot
Copy link
Owner

danielridgebot commented Jul 1, 2024

Hi @andyphancode this is Roslyn Wythe logged into danielridgebot. Please advise - I don't understand how the version of github-pages gem in ghpages-docker could be up to date. Nothing in that repo, including Dockerfile has been updated in 2 years. When a new image is created, it is uploaded to hackforlaops/ghpages which shows that the image was last pushed 2 years ago.

@andyphancode
Copy link
Contributor Author

andyphancode commented Jul 1, 2024

@danielridgebot Thanks for inquiring.

As far as I understand, the two relevant workflows in the Daniel Ridge Repo run daily and are intended to perform the following:

  1. Find and set the current github-pages gem/Ruby version from the Daniel Ridge Repo respectively
  2. Make a fetch request to versions.json on GitHub Pages to check for their current up-to-date versions. Note that this means if GitHub Pages doesn't change their dependency versions for 2 years then we'd be up-to-date for 2 years.
  3. Write the most up-to-date version to ruby.txt or github-pages-gem.txt
  4. If the new version on either .txt file differs from the original repository's version, then it'll be caught when the workflow checks for modified files in the repository
  5. If caught, the new .txt file is committed and an issue is automatically opened in hackforla/ops issues with the title "GHPAGES-DOCKER needs to be updated" and then whoever takes on the issue will update the ghpages-docker repo as necessary

It seems that since GitHub Pages hasn't updated their required Ruby version on versions.json in 2 years, there hasn't been an issue opened or need to update it since then.

Since investigating this inquiry, I actually noticed two issues that might be of concern:

  1. The Daniel Ridge Repo detected a change in github-pages-gem versions on the 5/14/2024 workflow but failed and exited due to errors. A commit was still made however on github-pages-gem.txt. I'm not quite sure about the history of this occurrence but assume somebody saw the error and made a manual commit to update the version. An issue was also not posted on hackforla/ops issues confirming that this workflow experienced an error, as I cannot find any issues titled "GHPAGES-DOCKER needs to be updated" from that time period, and based on this information I'm concerned that future automated detections will behave the same way and not work for both workflows.
  2. The occurrence beforehand would suggest that the ghpages-docker repo would be out of date but according to the ghpages-docker wiki, the github-pages gem updates itself to the latest version automatically whenever the image is rebuilt. I believe this would suggest that the github-pages gem workflow of the Daniel Ridge Bot is completely unnecessary and could be removed, as we see it failed anyways yet had no consequence on the ghpages-docker repo.

Update on previous comment:

I went ahead and tested .github/workflows/check-gh-pages-version.yml by just putting an older version in github-pages-gem.txt (not sure why I didn't think of that earlier) and managed to have a successful workflow run and issue created so I checked off the final task on the original issue.

success2

I'm still wary about concern #1 since the code for pushing a commit is unchanged. Image below is the error associated with the linked workflow in concern #1. From my forked repository, the workflow successfully pushed a commit to update my github-pages-gem.txt from 227 to 231 and then create an issue so I would assume its fixed but I failed to understand how this error on the main repository occurred in the first place.

fail

I'm not sure if this concern is out of the scope of the original issue butt I could suggest a code change from:

git config --global user.name 'Daniel Ridge'
git config --global user.email '[email protected]'
git commit -am "New release version"
git push

to:

git config --global user.name 'Daniel Ridge'
git config --global user.email '[email protected]'
git pull --rebase
git commit -am "New release version"
git push --force-with-lease

to potentially circumvent this error. Let me know if I should add it or if it should be delegated to a different future issue.

@roslynwythe
Copy link

roslynwythe commented Jul 3, 2024

Thank you @andyphancode for providing that information and for performing additional testing

  1. In the 5/14/2024 workflow run, a discrepancy in version numbers was detected (PAGES_RELEASE_VERSION=231 vs PAGES_CURRENT_VERSION=227) however the issue creation failed, which I think may have been due to a token issue. See Add Keepalive Workflow to Check GH Pages Version Workflow #21 (comment). In your opinion, Is that consistent with the log?
  2. I believe the commit you noted (of release-versions/github-pages-gem.txt) was not manual but was from line 34 of check-github-pages-gem-version. Based on that file, the workflow is now incorrectly concluding that both versions are 231 which is not accurate. In fact the Docker image has not been updated in 2 years and according to the release history of github-pages-gem, version 227 appears to be correct.
  3. I wonder if in this PR we should update manually update release-versions/github-pages-gem.txt with the version 227? Is there a more reliable way for the workflow to determine the version of the package in the Docker image? What if the Dockerfile explicitly set a version number and also stored that version number in a file so it can be referenced by DanielRidge workflows?

@andyphancode
Copy link
Contributor Author

Hi @roslynwythe ,

Thanks for providing some context to the history of this issue.

The events of the logs seem consistent. It seems that the workflow failed to run completely as a result of a token issue (May 14 12:06 PDT) but the workflow at least made it through with the code to push this commit thus updating the release version from 227 to 231. It was reran later in Jun and failed due to some git push errors but seems to not have had an issue since in following automated workflows.

I'm a bit confused regarding points 2 and 3 as I don't understand how 227 is the correct version as the order of release version information is going from GitHub Pages -> DanielRidge -> our Dockerfile image and the point of DanielRidge is to keep our image updated to the current GitHub Pages version which is 231.

From my understanding the DanielRidgeBot repo is making a fetch request to GitHub Pages versions.json to check on ruby and github-pages versions which currently labels github-pages as "231" as the most up-to-date github-pages gem dependency. This is consistent with the release history + current version in their file both being '231'.

As such, whenever GitHub Pages decides to update their version dependencies on versions.json, DanielRidgeBot should detect it in the next workflow and 1) update release-versions/github-pages-gem.txt and/or release-versions/ruby.txt with the new version then follow it up with 2) automatically creating an issue on ghpages-docker indicating an update is needed so somebody can update the Dockerfile on ghpages-docker manually. As a result, I don't see how '227' is the correct version.

It appears this process occurred on May 14, causing step 1 to happen (release-versions/github-pages-gem.txt got changed from 227 to 231) but token errors prevented step 2 from happening. Despite an issue not being created, having a manual change on the Dockerfile wasn't entirely necessary since the latest version on versions.json is used anyways whenever the image is built. It may still be important to create an alert via issue creation however, since a new version may suggest a new image should be built manually so that it automatically updates with the new github-pages-gem version. For the Ruby version, it was last manually updated 2 years ago in August 2022 and also reverted temporarily to test DanielRidge back in Oct 2022 as seen here. Since then, the Ruby version on versions.json has remained 2.7.4 thus the Dockerfile hasn't changed or required a change since 2 years ago.

Let me know if I am misunderstanding something regarding this issue.

@roslynwythe
Copy link

roslynwythe commented Jul 7, 2024

@andyphancode The terminology is a bit confusing, but I'm saying that the version of github-pages-gem currently used in our Docker image is 227 which is out of date (since the most recent release, given by PAGES_RELEASE_VERSION is 231), however the DanielRidge workflow .github/workflows/check-gh-pages-version.yml is not detecting that discrepany, and is not going to create issues alerting us to update the Docker file, because the workflow retrieves the current version (PAGES_CURRENT_VERSION) from release-versions/github-pages-gem.txt which now has an incorrect value of 231. Again, I'm saying it is incorrect because the Docker file has version 227.

I agree that when a new image is built, it will automatically update with the new version. I was hoping that the DanielRidge workflow would generate the hackforla/ops issue to update/upload the Docker file, but as discussed above, that will not happen with the current PR. I was suggesting that by manually editing release-versions/github-pages-gem.txt, we could force the workflow to generate the required issues. Another option would be to manually generate an issue in hackforla/ops to update/upload the Docker file.

@andyphancode
Copy link
Contributor Author

@roslynwythe

I see, this is clear to me now, thank you for the clarification in this otherwise confusing issue! I agree that the easiest way would be to change the current version to 227, so I will add that to the PR.

Let me know if there are any other concerns.

@danielridgebot danielridgebot self-requested a review August 11, 2024 16:41
@danielridgebot
Copy link
Owner

danielridgebot commented Aug 11, 2024

Thank you @andyphancode . Please resolve the merge conflict ,then let me know via Slack. Based on our previous discussion I believe you should retain the line "227".

Copy link
Owner

@danielridgebot danielridgebot left a comment

Choose a reason for hiding this comment

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

Great job @andyphancode updating the versions and syntax for checking for modified files in this PR. Thanks also for updating github-pages-gem.txt so that the workflow will generate an issue as intended.

@danielridgebot danielridgebot merged commit 88c4a2c into danielridgebot:main Aug 14, 2024
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.

3 participants