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

feat: Refactor wait-for-required-workflows #9929

Closed
1 task done
candiduslynx opened this issue Apr 13, 2023 · 3 comments
Closed
1 task done

feat: Refactor wait-for-required-workflows #9929

candiduslynx opened this issue Apr 13, 2023 · 3 comments
Assignees

Comments

@candiduslynx
Copy link
Contributor

candiduslynx commented Apr 13, 2023

Which problem is this feature request solving?

I think the wait-for-required-workflows script might require an overhaul to accommodate for #9655.

I wonder if we could move the script for wait from the workflow to the separate file (easier diff, dev): https://github.com/marketplace/actions/github-script#run-a-separate-file

Additionally, I think we could benefit from splitting the logic of the wait script into distinct parts (this can be doe either together with moving the code to separate file(s) or by splitting script step into several):

  • define the list of actions to wait for
  • actual waiting

Warning
The previous CI issue (#9388) took a couple of weeks to get resolved.

Note
Currently only @erezrokah has the expertise required to review the changes.
Additionally, the changes have to be thoroughly tested.
This makes this issue a slow one to implement as well as review.

Describe the solution you'd like

  1. Move the logic to a separate JavaScript file
  2. Split the functionality from single script into several scoped scripts
  3. Introduce skip-is-ok logic for reused steps as this allows not to use runner at all and thus, save some CPU cycles/money, too

Pull request (optional)

  • I can submit a pull request
@erezrokah
Copy link
Contributor

Thanks for opening the issue @candiduslynx. The changes described in this issue makes sense to me, and they are low risk and should be fast to implement, specifically:

Describe the solution you'd like

  1. Move the logic to a separate JavaScript file
  2. Split the functionality from single script into several scoped scripts
  3. Introduce skip-is-ok logic for reused steps as this allows not to use runner at all and thus, save some CPU cycles/money, too

As for #9655, the bulk of that change (and the risky part) is described in #9655 (comment). While the refactoring can help with making that change, I'm not sure it will reduce the risk or complexity of it. Regardless we don't need to couple those and can still do the refactoring, WDYT?

@candiduslynx
Copy link
Contributor Author

candiduslynx commented Apr 16, 2023

@erezrokah your summary is on point.
I implemented moving to a separate file & split in #10072, and I think it's ready to be merged (at the very least, reviewed).
As for the skip part - I'd like to add it in a separate PR to make it a smaller change.

kodiakhq bot pushed a commit that referenced this issue Apr 16, 2023
…#10072)

Part of #9929

Inspired by https://github.com/marketplace/actions/github-script#run-a-separate-file-with-an-async-function

The code is moved to a couple of separate js files & steps:
1. Define the required actions
2. Perform actual waiting

Tested #10120
@yevgenypats
Copy link
Member

Closing given low priority. if ever becomes a high priority we will re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants