Skip to content

github actions: Use python PR check script - LE-2214 #82

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

Closed
wants to merge 1 commit into from

Conversation

gvrose8192
Copy link

@gvrose8192 gvrose8192 commented Jan 17, 2025

jira LE-2214

Obsoletes the old ruby PR check script

Drop in replacement for the old ruby coded PR checker. The LE-2214 task has changed underneath us and we no longer are looking for a PR checker that works with forked repos. The current method is to accept a PR from a forked repository and then a CIQ employee will run the PR checker after having vetted the code to make sure it is acceptable - especially making sure that the PR from the forked repo contains no changes to any github actions.

Here's an example run: https://github.com/ctrliq/kernel-src-tree/actions/runs/12836125278

You'll see that the PR checker for this PR failed, because the PR checks are from the target and not the source. Plz ignore that for this PR - I posted the results of this PR checker running above.

And just to reiterate - you can't use a PR checker to check a new PR checker. It's like looking in a mirror of a mirror of a mirror... ad infinitum. :D

jira LE-2214

Obsoletes the old ruby PR check script
print(s)
file.write(s)
file.close()
return retcode
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think retcode is still 0 here (which is success I think), but in the ruby code a non-ciq email address was a failure.

BUT, we are going to have non-ciq authors now, right? so maybe we don't want this check at all?

Copy link
Author

@gvrose8192 gvrose8192 Jan 17, 2025

Choose a reason for hiding this comment

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

Yeah, that will need fixing. It was an assumption that is no longer true.
Edited to state clearly that yes, I'm just returning a pass code on a non-ciq email address.

Comment on lines +90 to +107
first_arg, *argv_in = sys.argv[1:] # Skip script name in sys.argv

if len(argv_in) < 5:
print("Not enough arguments: fname, target_branch, source_branch, prj_dir, pull_request, requestor")
sys.exit()

fname = str(first_arg)
fname = "tmp-" + fname
# print("filename is " + fname)
target_branch = str(argv_in[0])
# print("target branch is " + target_branch)
source_branch = str(argv_in[1])
# print("source branch is " + source_branch)
prj_dir = str(argv_in[2])
# print("project dir is " + prj_dir)
pullreq = str(argv_in[3])
# print("pull request is " + pullreq)
requestor = str(argv_in[4])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very bash we could use argparse like so to make sure the switches have what they need.
https://github.com/ctrliq/kernel-src-tree-tools/blob/mainline/ciq-cherry-pick.py#L13-L21

Copy link
Author

Choose a reason for hiding this comment

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

You'll get no argument from me. 😁 OK, let me fix that up.

upstream_diffdiff_sha = ""
upstream_diff = False

for logline in loglines:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't touched this is a while but I'm going to start looking at it again for the curent tiger team I'm on,
What are your thoughts about leveraging this and putting this process-git-request.py into kernel-src-tree-tools and check that repo out along side kernel-src-tree
https://github.com/ctrliq/kernel-tools/blob/master/ciq_helpers.py#L12-L47

print(f"diffdiff err: " + diff_err)
retcode = 1
file.write("error:\nCommit: " + local_diffdiff_sha + " differs with no upstream tag in commit message\n")

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're going to close the file here before returning we should do the same for consistency ... even though the file is closed when the scope of the object is released
a9eeb8d#diff-675bae9c01428bfeaef67c1d6366e76b5eba0bb631b387036dd57c6ba35b97ddR53

@gvrose8192
Copy link
Author

I'm going to close this PR request - redo it substantially and then repost when I like it better.

@gvrose8192 gvrose8192 closed this Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants