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

fix: Honnor booting instance in runner pool #2801

Merged
merged 8 commits into from
Jan 12, 2023

Conversation

npalm
Copy link
Collaborator

@npalm npalm commented Dec 28, 2022

@mckern @M1kep Thanks for the work done in #2590 and #2733

When digging in today for review I found out the test are not running as expected. Bu fixing th test I decided also to take the changes you proposed in your PR into account. I would appreciate when you have a bit of time to check this PR.

@npalm
Copy link
Collaborator Author

npalm commented Dec 28, 2022

@mckern @M1kep please can you have a look on the PR. I added tests and refactory the implementaiton so both registered instances as still booting instances are considered.

@mckern
Copy link
Contributor

mckern commented Dec 28, 2022

Oh, thank you! I have been working this on the clock (and therefore away from my work computer for the last week and a half); I didn't mean to leave it fallow as long as I did but the end of the year is always kind of a liminal time at work. I'll take a look when I'm on the clock tomorrow (Wednesday) -- this was much appreciated.

@npalm
Copy link
Collaborator Author

npalm commented Dec 28, 2022

@mckern @M1kep please can you have a look on the PR. I added tests and refactory the implementaiton so both registered instances as still booting instances are considered.

Oh, thank you! I have been working this on the clock (and therefore away from my work computer for the last week and a half); I didn't mean to leave it fallow as long as I did but the end of the year is always kind of a liminal time at work. I'll take a look when I'm on the clock tomorrow (Wednesday) -- this was much appreciated.

No problem, had a bit of time and thought let's dig in. I hope this helps. I will leave the PR open. Hopefully you have a bit of time early next year to check the implementation.

@npalm npalm force-pushed the fix/respect-booting-instances-pool branch from 194ea63 to 1d42c41 Compare December 28, 2022 11:36
@npalm npalm changed the base branch from develop to main December 28, 2022 23:05
mckern
mckern previously approved these changes Jan 4, 2023
@npalm
Copy link
Collaborator Author

npalm commented Jan 5, 2023

@M1kep thanks for the comments, will check asap

M1kep and others added 5 commits January 11, 2023 08:14
Rebased off of the patch provided by Michael Poutre
<[email protected]> in
philips-labs/terraform-aws-github-runner#2590 and re-targeted against
the develop branch.

Co-authored-by: Ryan McKern <[email protected]>
I had a very different version of prettier installed globally; running
`yarn run prettier --write` should hopefully be enough to get this
fixed up and passing.
@npalm npalm force-pushed the fix/respect-booting-instances-pool branch from 03f22d6 to 8a3984b Compare January 11, 2023 07:25
@npalm npalm requested review from M1kep and mckern and removed request for M1kep and mckern January 11, 2023 08:05
@npalm npalm requested a review from mckern January 11, 2023 08:05
@npalm npalm merged commit 9f841f7 into main Jan 12, 2023
@npalm npalm deleted the fix/respect-booting-instances-pool branch January 12, 2023 12:27
@npalm npalm restored the fix/respect-booting-instances-pool branch January 12, 2023 12:31
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