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

Comma-separated patterns for conclusion & state; remove 'all' from delete_workflow_pattern; various tweaks #16

Merged
merged 8 commits into from
Oct 28, 2023

Conversation

God-damnit-all
Copy link
Contributor

  1. delete_run_by_conclusion_pattern can now match multiple conclusions by using a comma-separated list
  2. delete_workflow_by_state_pattern can now match multiple states by using a comma-separated list
  3. Removes 'all' from delete_workflow_pattern since it's unnecessary and makes it unable to target workflows named 'all'
  4. Updates the manual triggered workflow example to include an 'Unsuccessful' option for delete_run_by_conclusion_pattern
    a. uses a ternary expression within its job step parameter to resolve to action_required,cancelled,failure,skipped
  5. Various description and formatting tweaks

Originally I was only going to do the first item on the above list, but then I figured I should also apply it to the second item, and then I kept noticing other, smaller things that bothered me.

I organized all the changes by commit the best I could.

@God-damnit-all
Copy link
Contributor Author

God-damnit-all commented Oct 24, 2023

Just did a force-push because I realized my text editor was trimming the 2 soft-line-breaks I added to README.md

@Mattraks Mattraks merged commit 998b892 into Mattraks:main Oct 28, 2023
@kamilsk
Copy link

kamilsk commented Oct 30, 2023

Hi! Changes related to delete_workflow_pattern breaks compatibility. It requires extra code writing in a case with Options. E.g., withsparkle/service@9310d31

Context: https://github.com/withsparkle/service/blob/9310d31a9d1e1115d505bb7d74afd350fdd5040e/.github/workflows/cleanup.runs.yml#L9-L33.

@zydou
Copy link

zydou commented Nov 16, 2023

can anyone tell me why the following configuration delete all my workflow runs (including cancelled, failure, skipped and success)?

      - name: Delete runs by conclusion
        uses: Mattraks/delete-workflow-runs@v2.0.5
        with:
          retain_days: 0
          keep_minimum_runs: 0
          delete_run_by_conclusion_pattern: cancelled

BTW, v2.0.4 works fine.

@God-damnit-all
Copy link
Contributor Author

God-damnit-all commented Nov 16, 2023

can anyone tell me why the following configuration delete all my workflow runs (including cancelled, failure, skipped and success)?

      - name: Delete runs by conclusion
        uses: Mattraks/delete-workflow-runs@v2.0.5
        with:
          retain_days: 0
          keep_minimum_runs: 0
          delete_run_by_conclusion_pattern: cancelled

BTW, v2.0.4 works fine.

Uh, that is a very good question. The if check was altered from

        if (delete_run_by_conclusion_pattern && delete_run_by_conclusion_pattern.toLowerCase() !== "all"
            && run.conclusion.indexOf(delete_run_by_conclusion_pattern) === -1  ) {

to

        if (delete_run_by_conclusion_pattern
            && delete_run_by_conclusion_pattern.split(",").map(x => x.trim()).includes(run.conclusion)
            && delete_run_by_conclusion_pattern.toUpperCase() !== "ALL") {

I don't understand how that could somehow be resolving to true ...

@zydou
Copy link

zydou commented Nov 16, 2023

This is a BREAKING CHANGE and should bump the tag to v3, because people use Mattraks/delete-workflow-runs@v2 will lost all history runs. @Mattraks

@God-damnit-all
Copy link
Contributor Author

Well, I figured out the problem. I am the biggest dumbass in the universe and I feel terrible. It's not a breaking change, it's just a bug that wormed its way in when I was reformatting the code. The condition is supposed to be inverted.

So it's deleting every run EXCEPT "cancelled" because I accidentally deleted a single exclamation point.

I am really sorry, everyone.

@God-damnit-all
Copy link
Contributor Author

#17

For what it's worth, I promise you all that I'm going to feel really shitty that I did this for a while.

@zydou
Copy link

zydou commented Nov 16, 2023

It's really okay, we all make mistakes. Anyway, thanks for your effort to improve this action. 🎉

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.

4 participants