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

Optionally search for workflows #270

Merged
merged 3 commits into from Feb 15, 2024
Merged

Optionally search for workflows #270

merged 3 commits into from Feb 15, 2024

Conversation

romangg
Copy link
Contributor

@romangg romangg commented Feb 14, 2024

When no workflow is set, until now the current workflow has been chosen. But the workflow we get the artifact from could be another one determined by other criteria like the branch and the artifact name.

We provide a new option workflow_search, that if set, allows us to search for the workflow repo-wide according to the other provided data.

This is a failing job with upstream action v3: https://github.com/romangg/theseus-ship/actions/runs/7904208512/job/21573949899

This is a successful job ran against the version from this PR: https://github.com/romangg/theseus-ship/actions/runs/7906156620/job/21581226467

In my case the workflows are named differently depending on them being a scheduled workflow or a workflow on push.

This new option defaulting to false should be compatible with current usage of the action.

@romangg romangg marked this pull request as draft February 14, 2024 16:53
The API is now versioned. The link has changed.
When no workflow is set, until now the current workflow has been chosen. But
the workflow we get the artifact from could be another one determined by other
criteria like the branch and the artifact name.

We provide a new option workflow_search, that if set, allows us to search for
the workflow repo-wide according to the other provided data.
@romangg romangg changed the title Query all repo runs to not rely on run id being provided Optionally search for workflows Feb 14, 2024
@romangg romangg marked this pull request as ready for review February 14, 2024 19:43
@dawidd6
Copy link
Owner

dawidd6 commented Feb 15, 2024

Nice, thanks!
Let's see if anything breaks 😄.

@dawidd6 dawidd6 merged commit f6b0bac into dawidd6:master Feb 15, 2024
16 checks passed
@etripier
Copy link

@romangg @dawidd6 Hello, it looks like the latest version of this action is exhausting our GitHub rate limit. From a very cursory look it seems like you're getting all workflow runs in the current repo if the immediate workflow run is unable to be found, and then checking the workflow run against the GitHub API for every run in said repo. That's a lot of requests.

@dawidd6
Copy link
Owner

dawidd6 commented Feb 15, 2024

@etripier @romangg care to take a look and provide some PR? If not, maybe we should revert?

@etripier
Copy link

I don't have time, unfortunately. But a revert might be the safest option.

@romangg
Copy link
Contributor Author

romangg commented Feb 16, 2024

I can take a look later today. So the issue is that listWorkflowRunsForRepo is too heavy, right?

That should only be called though when no workflow was provided. Something that shouldn't have worked before anyway. Can I take a look at your repo @etripier?

@romangg
Copy link
Contributor Author

romangg commented Feb 16, 2024

@etripier Can you try #271? It restores a parameter to the request that is the commit which got moved to the body for no good reason. If you in your pipelines provided the commit value, that might be the reason why you're thitting the rate limit and it's not about the new listWorkflowRunsForRepo after all.

@romangg romangg deleted the fix-branch branch February 16, 2024 17:12
main.js Show resolved Hide resolved
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.

None yet

4 participants