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

Exclude workflows for changes to Markdown files #4192

Merged
merged 1 commit into from
Aug 31, 2024

Conversation

yash-zededa
Copy link
Collaborator

@yash-zededa yash-zededa commented Aug 30, 2024

Summary

In this pull request, I have implemented the following changes:

  1. Added paths-ignore Configuration:

    • Updated the workflow configuration to include paths-ignore for Markdown file changes. This adjustment ensures that the build and test processes are skipped when only Markdown files are modified.
  2. Revised get_run_id Implementation:

    • Refactored the get_run_id function to utilize the GitHub API instead of relying on the previous GitHub script. This change improves the reliability and efficiency of retrieving the run ID by leveraging GitHub's API directly.

@OhmSpectator
Copy link
Member

You have a very nice PR description!
Please put it into the commit message =)

@yash-zededa yash-zededa marked this pull request as draft August 30, 2024 16:46
@yash-zededa
Copy link
Collaborator Author

You have a very nice PR description! Please put it into the commit message =)

Yup, Doing that. Fixing yetus issues as well.

@OhmSpectator
Copy link
Member

Also, the two parts of the change desire to be separate commits.

@yash-zededa yash-zededa force-pushed the skip-wf-for-mdfiles branch 2 times, most recently from 7b070d9 to c6d65db Compare August 30, 2024 17:11
@OhmSpectator
Copy link
Member

By the two commits, I meant those:

1st:
Updated the workflow configuration to include paths-ignore for Markdown
file changes. This adjustment ensures that the build and test processes
are skipped when only Markdown files are modified.

2nd:
Refactored the get_run_id function to utilize the GitHub API instead of
relying on the previous GitHub script. This change improves the
reliability and efficiency of retrieving the run ID by
leveraging GitHub's API directly.

@yash-zededa
Copy link
Collaborator Author

By the two commits, I meant those:

1st: Updated the workflow configuration to include paths-ignore for Markdown file changes. This adjustment ensures that the build and test processes are skipped when only Markdown files are modified.

2nd: Refactored the get_run_id function to utilize the GitHub API instead of relying on the previous GitHub script. This change improves the reliability and efficiency of retrieving the run ID by leveraging GitHub's API directly.

ok, both those are not 2 commits but like 2 points. I agree with the idea of representation, I will amend the commit message.

thanks.

@yash-zededa yash-zededa force-pushed the skip-wf-for-mdfiles branch 3 times, most recently from fac49d7 to 859cb57 Compare August 30, 2024 17:29
@yash-zededa yash-zededa marked this pull request as ready for review August 30, 2024 17:31
Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Approving to test the changes in the workflow

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

The script fails now:

Run echo "github context is {
/home/runner/work/_temp/7eab4ed2-b1cb-49c4-a429-cd25e80cd8f3.sh: line 236: unexpected EOF while looking for matching `''
Error: Process completed with exit code 2.

@yash-zededa
Copy link
Collaborator Author

The script fails now:

Run echo "github context is {
/home/runner/work/_temp/7eab4ed2-b1cb-49c4-a429-cd25e80cd8f3.sh: line 236: unexpected EOF while looking for matching `''
Error: Process completed with exit code 2.

Fixed it, printing the context directly with echo causes interpolation problems.

If the changes look good, please approve so we can proceed with testing.

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Approving to test the script again.
Also, please amend the fix to the main commit. One PR should not introduce a problem and solve it with the next commit =)

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

  RESPONSE=$(curl -s -L "https://api.github.com/repos/lf-edge/eve/actions/workflows/build.yml/runs?head_sha=5ded92778ce7b6d331ca1e3a4830db0137902e73&status=success" \
    -H "Authorization: ***" \
    -H "Accept: application/vnd.github+json")
  # Check if RESPONSE is empty
  if [ -z "$RESPONSE" ]; then
    echo "API call returned no response."
    exit 1
  fi
  RUN_ID=$(echo $RESPONSE | jq -r '.workflow_runs[0].id')
  # Check if the RUN_ID is empty
  if [ "$RUN_ID" == "null" ] || [ -z "$RUN_ID" ]; then
    echo "No successful runs found"
    exit 1
  fi
  echo $RUN_ID
  echo "run_id=$RUN_ID" >> $GITHUB_OUTPUT
  shell: /usr/bin/bash -e {0}
  env:
    GITHUB_TOKEN: ***
No successful runs found

@yash-zededa
Copy link
Collaborator Author

  RESPONSE=$(curl -s -L "https://api.github.com/repos/lf-edge/eve/actions/workflows/build.yml/runs?head_sha=5ded92778ce7b6d331ca1e3a4830db0137902e73&status=success" \
    -H "Authorization: ***" \
    -H "Accept: application/vnd.github+json")
  # Check if RESPONSE is empty
  if [ -z "$RESPONSE" ]; then
    echo "API call returned no response."
    exit 1
  fi
  RUN_ID=$(echo $RESPONSE | jq -r '.workflow_runs[0].id')
  # Check if the RUN_ID is empty
  if [ "$RUN_ID" == "null" ] || [ -z "$RUN_ID" ]; then
    echo "No successful runs found"
    exit 1
  fi
  echo $RUN_ID
  echo "run_id=$RUN_ID" >> $GITHUB_OUTPUT
  shell: /usr/bin/bash -e {0}
  env:
    GITHUB_TOKEN: ***
No successful runs found

The error is anticipated because we included:

paths-ignore:
  - '.github/workflows/build.yml'

This configuration prevents the build.yml file from triggering the workflow, which is expected since we are using pull_request_target. However, I'll implement a conditional check to ensure that if changes involve build.yml, the eden.yml file will be ignored.

@yash-zededa
Copy link
Collaborator Author

Approving to test the script again. Also, please amend the fix to the main commit. One PR should not introduce a problem and solve it with the next commit =)

I've tested the workflows on the test repositories, but there are a few conditions that might have been overlooked since lf-edge/eve has a more complex setup. Once everything is confirmed to be working correctly, I'll squash the commits and include a detailed, descriptive commit message. Hope this is fine :)

@OhmSpectator
Copy link
Member

However, I'll implement a conditional check to ensure that if changes involve build.yml, the eden.yml file will be ignored.

Do you have an idea how to test this PR without merging it and creating a new test PR on top?...

@yash-zededa
Copy link
Collaborator Author

However, I'll implement a conditional check to ensure that if changes involve build.yml, the eden.yml file will be ignored.

Do you have an idea how to test this PR without merging it and creating a new test PR on top?...

We don't need to merge it to test it. Only approval should be fine. I have tested in on the test repo and it's working fine.

Pushing changes in few mins.

@OhmSpectator
Copy link
Member

Pushing changes in few mins.

Only if you have nothing to do)) Otherwise, it definitely waits until Monday)

@yash-zededa
Copy link
Collaborator Author

Pushing changes in few mins.

Only if you have nothing to do)) Otherwise, it definitely waits until Monday)

Just want to wrap this up. We're unnecessarily triggering eden.yml for PR docs.

Pushed. @OhmSpectator, please approve—hopefully for the last time! 😊

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Approve to test

@yash-zededa
Copy link
Collaborator Author

yash-zededa commented Aug 31, 2024

Approve to test

Thanks, it seems to be working as expected now. I’ve already tested the behavior when the changes don’t include .md files or build.yml.

@OhmSpectator
Copy link
Member

Don't forget to make your commit message the cherry on top! You don't need to mention intermediate issues and fixes.

Updated the workflow configuration to include paths-ignore for Markdown
file changes. This adjustment ensures that the build and test processes
are skipped when only Markdown files are modified.

Refactored the get_run_id function to utilize the GitHub API instead of
relying on the previous GitHub script. This change improves the
reliability and efficiency of retrieving the run ID by
leveraging GitHub's API directly

Added a condition to trigger the `check_md_files` job only when the PR
is approved.

Updated check_md_files to include a check for build.yml. This ensures
that the job checks for changes to the build.yml file, preventing the
eden.yml workflow from failing if the PR contains changes exclusively to
build.yml.

Signed-off-by: yash-zededa <yash@zededa.com>
@yash-zededa
Copy link
Collaborator Author

yash-zededa commented Aug 31, 2024

Don't forget to make your commit message the cherry on top! You don't need to mention intermediate issues and fixes.

Since it's working as expected, I’ve updated the commit message to include the relevant details and removed the intermediate fixes.

thanks @OhmSpectator with the approvals.

@OhmSpectator
Copy link
Member

@yash-zededa, don't we backport it to the stable branches?

@OhmSpectator OhmSpectator merged commit 9306726 into lf-edge:master Aug 31, 2024
9 checks passed
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.

2 participants