-
Notifications
You must be signed in to change notification settings - Fork 0
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
Correct getting affected projects in Nx #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I don't see any issues
@@ -35,6 +35,8 @@ def fetch_remote_git_history(ref): | |||
print("No merge instance branch found. Exiting.") | |||
sys.exit(1) | |||
|
|||
write_to_github_output(f"merge_instance_branch={merge_instance_branch}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just logging, essentially?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, which is called during the prerequisite step of this workflow, is outputting to github's output, which gets passed around between all of the remaining steps in the flow. That's why we switch to passing in steps.prerequisites.outputs.merge_instance_branch
for the merge branch for the other steps (link). We do this because providing the merge branch to us is optional - we use the repo's default branch if it isn't provided, and we make the prerequisite step figure that out for us (link)
|
||
affected_json = json.loads(run_command(f"cat {affected_json_out}", return_output=True)) | ||
affected_list_out = f"./{merge_instance_branch_head_sha}_{pr_branch_head_sha}.txt" | ||
nx_show_command_base = f"npx nx show projects --affected --base={merge_instance_branch_head_sha} --head={pr_branch_head_sha}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this out locally and it seems to check out.
Context - the current Nx action didn't work for Nx versions above 19 - this corrects that
Proof of it working with targets -https://github.com/trunk-io/mergequeue-nx/actions/runs/10638401563?pr=207, for PR https://github.com/trunk-io/mergequeue-nx/pull/207
Proof of it working with no targets - https://github.com/trunk-io/mergequeue-nx/actions/runs/10638161072/job/29493542313?pr=208, for PR https://github.com/trunk-io/mergequeue-nx/pull/208
Proof of it working with an impacts all diff - https://github.com/trunk-io/mergequeue-nx/pull/209, for PR https://github.com/trunk-io/mergequeue-nx/pull/209