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

modified: src/automerge.py #5

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

F-WRunTime
Copy link
Member

@F-WRunTime F-WRunTime commented Nov 27, 2024

  • Simplified Check: The updated code retrieves the combined status of the latest commit directly.
  • Single Condition: It checks if the combined status state is 'success' to determine if all checks have passed.
  • Logging: It logs a single message if the checks are not passing, indicating the overall state.

Benefits of the Change:

  • Efficiency: The new approach reduces the complexity by avoiding the need to manually iterate over each required check.
  • Readability: The code is more concise and easier to understand, focusing on the overall status rather than individual checks.
  • Performance: By using the combined status, it potentially reduces the number of API calls and processing required.
  • This change streamlines the process of determining if a PR is ready to be merged based on its status checks, making the code more maintainable and efficient.

Minimum requirements
image
image
image
image

- Modify api end points used to check status. Previous did not have a
  permission option with fine-grained tokens.
- This reduces permissioni requirements to use automerger
- Update tests to use the new less permissive token.
- Checking the 'chech' statuses per commit
- Ccombined commit was not
  always accurate to the mergeable state of  a PR due to annotations and
other items checked not important to basic check passed? Approved? Up to
datae? Yes? Merge.
src/automerge.py Outdated
all_checks_passed = False
commit = pr.get_commits().reversed[0]
statuses = commit.get_statuses()
all_checks_passed = all(print(status) for status in statuses)

Choose a reason for hiding this comment

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

This evaluates to False regardless of statuses. (And as a side effect, will print the first status.)

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests came back as expected.
https://github.com/runtimeverification/automerge/actions/runs/12061856900/job/33634749102#step:4:1

I'll debug this now. Must be getting a false positive

Copy link
Member Author

@F-WRunTime F-WRunTime Dec 3, 2024

Choose a reason for hiding this comment

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

Addressed this issue. This required reworking permissions to introduce the use for a Github App.
Remaining is to update the README and docs and tests

- Rework logic.
- New dependency for infrastructure to run. Install Github App
  AutomergePR Permission Control to have access to check statuses. Bug
in Github fine-grained tokens or deprecated documentaiton never removed.
- Updating workflow test to use the GH APP credentials
modified:   README.md
- Modifying instructions and informating need for github and what
  permissions are needed for the github App
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants