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

on event push without PRs fallback to 0 #354

Closed
fulgas opened this issue Nov 6, 2024 · 13 comments · Fixed by #358
Closed

on event push without PRs fallback to 0 #354

fulgas opened this issue Nov 6, 2024 · 13 comments · Fixed by #358
Assignees

Comments

@fulgas
Copy link

fulgas commented Nov 6, 2024

Is your feature request related to a problem

When there is a push to the main branch and there is no PR associated with it, the PR Number is null.

Describe the solution you'd like

Is it possible to use a fallback to 0 ?

Describe alternatives you've considered

Additional context

@rdhar
Copy link
Member

rdhar commented Nov 6, 2024

Hey @fulgas, TF-via-PR explicitly supports both pull_request and push event triggers, shown in the Readme and this example workflow.

This includes fetching the PR number associated with a commit, even from push trigger.

What's more, TF-via-PR also has a fallback of 0 if it can't determine the PR number from various query methods.

Are you able to share a little more about your workflow, with (obfuscated) snippets to aid troubleshooting.

@fulgas
Copy link
Author

fulgas commented Nov 6, 2024

I'm running a plan when i push directly to the main branch.

This is the identifier for the file terraform.null...chdir...tf.plan.

The ${pr_number} is null and since it fall into the if [[ "$GITHUB_EVENT_NAME" == "push" ]]; im assuming that it didnt found any PR associated when the commit.

@rdhar
Copy link
Member

rdhar commented Nov 6, 2024

Hmm, that doesn't sit right. Could you:

  1. Bump up to the latest version devsectop/[email protected] and share the new plan file artifact name.
  2. Confirm if there is any commit present on the PR branch?

I'm apprehensive of adding in a fallback to "$GITHUB_EVENT_NAME" == "push", without first figuring out why it failed to return a number. Specifically:

gh api /repos/${GITHUB_REPOSITORY}/commits/${GITHUB_SHA}/pulls

@fulgas
Copy link
Author

fulgas commented Nov 6, 2024

This is not your typical workflow as you showed on the examples.

I have 2 workflows, one for CI which runs on PR and produces the tf plan, and another workflow for CD that actually applies the plan.

The difference here is that besides that i am allowing to push directly to the main branch even when no PR is associated.

@fulgas
Copy link
Author

fulgas commented Nov 6, 2024

The result is the same: terraform-null-3eee8c84341b95e00d4d4dd1307a8268.tfplan

I can try to play with label-pr and comment-pr to none when i know that the push is not related to a PR on anyway.

@fulgas
Copy link
Author

fulgas commented Nov 6, 2024

gh api /repos/${GITHUB_REPOSITORY}/commits/${GITHUB_SHA}/pulls

This call returns [] which is true and a validate response in this case.

@rdhar
Copy link
Member

rdhar commented Nov 6, 2024

Admittedly, I haven't yet run into the use case of having a CI workflow as well as another workflow which circumvents it altogether.

I'm afraid neither label-pr nor comment-pr will help in this case, since the PR number is a crucial component of TF-via-PR.

Before working on a solution, could you share more details around pipeline flow and expectations?

  • Do you have a pull_request trigger for the initial plan file generation and storage?
  • Upon push without PR, do you expect to retrieve and apply the plan file from a certain PR?
  • Or do you intend on a more direct apply -auto-approve approach instead?

@fulgas
Copy link
Author

fulgas commented Nov 6, 2024

This pipeline will run a plan then an apply.

  • Do you have a pull_request trigger for the initial plan file generation and storage?

No PR

  • Upon push without PR, do you expect to retrieve and apply the plan file from a certain PR?

No

  • Or do you intend on a more direct apply -auto-approve approach instead?

No auto approve.

@fulgas
Copy link
Author

fulgas commented Nov 6, 2024

I'm afraid neither label-pr nor comment-pr will help in this case, since the PR number is a crucial component of TF-via-PR.

Works as i expected if i apply on the plan and apply

comment-pr: "none"
label-pr: false

I can live with the pr_number being null.

@rdhar
Copy link
Member

rdhar commented Nov 6, 2024

No way, I didn't think that approach would've worked, but you figured out a resolution yourself?!

Is there anything else to help with?

@fulgas
Copy link
Author

fulgas commented Nov 7, 2024

All good. Thk for the help :)

@rdhar
Copy link
Member

rdhar commented Nov 7, 2024

Thank you for demonstrating a new use case! If the current identifier-naming strategy doesn't work out, feel free to drop us another message—happy to amend based on feedback.

@rdhar
Copy link
Member

rdhar commented Nov 8, 2024

Although, you've managed to solve this, others (i.e., me) might've been stuck.

And given the simplicity—just push to main—I think this use case should be accounted for without resorting to null combined with comment-pr: none and label-pr: false.

As per your suggestion, the PR number now falls back on 0 in this scenario, and no further need to disable comment-pr or label-pr.

Happy to share this has been shipped with v12.0.7, where your contribution has been credited!

@rdhar rdhar self-assigned this Nov 8, 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 a pull request may close this issue.

2 participants