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: Option to register runners on organization instead of repo #451

Merged
merged 59 commits into from
Nov 1, 2023

Conversation

pharindoko
Copy link
Contributor

@pharindoko pharindoko commented Oct 16, 2023

App.svelte

  • Add ui component
  • Add parameter to new app request
  • Add parameter to existing app request

Secrets.ts

  • Create new secret for runner registration level in cdk

Setup.lambda.ts

  • Add secret arn to setup lambda.ts
  • Determine registration level based on permissions for newapp
  • Determine registration level for existing app
  • Update secret value

Tokenretriever.lambda.ts

  • Add env variable for registration level / secret arn
  • Add new condition to register token from org (octokit)

Determine runner registration config command for:

  • ec2 linux(userdata)
  • ec2 windows(userdata)
  • fargate linux
  • lambda linux
  • lambda linux arm
  • codebuild linux
  • codebuild linux arm
  • ecs linux
  • ecs linux arm
  • ec2 linux arm
  • ...

Tests:

  • create new app with org level registration
  • create with existing app with org level registration
  • run default integration test to setup solution
  • run github action runners with all providers in default integration test in github.com

result:
image

closes #442

@pharindoko pharindoko force-pushed the ref/442-org-runner-registration branch from 5cb1ed9 to 7c6f65f Compare October 16, 2023 10:09
Copy link
Member

@kichik kichik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's still a draft, but I thought some early feedback might help.

setup/src/App.svelte Outdated Show resolved Hide resolved
setup/src/App.svelte Outdated Show resolved Hide resolved
src/providers/ec2.ts Outdated Show resolved Hide resolved
src/providers/ec2.ts Outdated Show resolved Hide resolved
src/secrets.ts Outdated Show resolved Hide resolved
src/setup.lambda.ts Outdated Show resolved Hide resolved
@kichik
Copy link
Member

kichik commented Oct 17, 2023

Determine runner registration config command for:

This might be easier if we do #452 first.

@pharindoko
Copy link
Contributor Author

pharindoko commented Oct 17, 2023

Determine runner registration config command for:

This might be easier if we do #452 first.

Yes makes sense. I can continue to work on the your findings in the meantime.

@pharindoko
Copy link
Contributor Author

pharindoko commented Oct 17, 2023

I know it's still a draft, but I thought some early feedback might help.

That was the intention. Thanks for your feedback.
Feel free to add changes directly. This PR affects way too much pieces that I can do it without your contribution.

@kichik
Copy link
Member

kichik commented Oct 27, 2023

I am seeing issues with specifically Fargate failing to update the runner version and exiting. It doesn't feel related, but I have to confirm. Have you seen something like this?

  | 2023-10-26T21:40:18.885-04:00 | Current runner version: '2.310.2'
  | 2023-10-26T21:40:18.886-04:00 | 2023-10-27 01:40:18Z: Listening for Jobs
  | 2023-10-26T21:40:28.438-04:00 | Runner update in progress, do not shutdown runner.
  | 2023-10-26T21:40:28.551-04:00 | Downloading 2.311.0 runner
  | 2023-10-26T21:41:11.211-04:00 | Waiting for current job finish running.
  | 2023-10-26T21:41:11.262-04:00 | Generate and execute update script.
  | 2023-10-26T21:41:11.306-04:00 | Runner will exit shortly for update, should be back online within 10 seconds.
  | 2023-10-26T21:41:11.315-04:00 | Runner update process finished.
  | 2023-10-26T21:41:11.847-04:00 | Runner listener exit because of updating, re-launch runner after successful update
  | 2023-10-26T21:41:42.021-04:00 | Restarting runner...
  | 2023-10-26T21:41:42.058-04:00 | /home/runner/run-helper.sh: line 36: /home/runner/bin/Runner.Listener: No such file or directory
  | 2023-10-26T21:41:42.058-04:00 | Exiting with unknown error code: 127
  | 2023-10-26T21:41:42.058-04:00 | Exiting runner...

- Remove unused RUNNER_LEVEL environment variable
- Quote REGISTRATION_URL everywhere to avoid issues with spaces
- Typos
@pharindoko
Copy link
Contributor Author

I am seeing issues with specifically Fargate failing to update the runner version and exiting. It doesn't feel related, but I have to confirm. Have you seen something like this?


  | 2023-10-26T21:40:18.885-04:00 | Current runner version: '2.310.2'

  | 2023-10-26T21:40:18.886-04:00 | 2023-10-27 01:40:18Z: Listening for Jobs

  | 2023-10-26T21:40:28.438-04:00 | Runner update in progress, do not shutdown runner.

  | 2023-10-26T21:40:28.551-04:00 | Downloading 2.311.0 runner

  | 2023-10-26T21:41:11.211-04:00 | Waiting for current job finish running.

  | 2023-10-26T21:41:11.262-04:00 | Generate and execute update script.

  | 2023-10-26T21:41:11.306-04:00 | Runner will exit shortly for update, should be back online within 10 seconds.

  | 2023-10-26T21:41:11.315-04:00 | Runner update process finished.

  | 2023-10-26T21:41:11.847-04:00 | Runner listener exit because of updating, re-launch runner after successful update

  | 2023-10-26T21:41:42.021-04:00 | Restarting runner...

  | 2023-10-26T21:41:42.058-04:00 | /home/runner/run-helper.sh: line 36: /home/runner/bin/Runner.Listener: No such file or directory

  | 2023-10-26T21:41:42.058-04:00 | Exiting with unknown error code: 127

  | 2023-10-26T21:41:42.058-04:00 | Exiting runner...

Hey @kichik,

no I haven't seen this message before. ...
But I haven't triggered and tested an update of a fargate runner by intention.

Does the same issue occur on the main branch ?

This will help with migrating from old versions
@kichik
Copy link
Member

kichik commented Oct 29, 2023

@pharindoko I think I'm done with my adjustments. The only thing I still want to do is test the setup function to see that it can still create apps after my changes for both repo and org levels. Let me know if it looks good to you too.

@pharindoko
Copy link
Contributor Author

pharindoko commented Oct 29, 2023

@pharindoko I think I'm done with my adjustments. The only thing I still want to do is test the setup function to see that it can still create apps after my changes for both repo and org levels. Let me know if it looks good to you too.

Have seen your changes.
I think it's fine.

Sorry that I missed to cleanup the runner level and check the formatting again.

The only thing I thought about is that it might be good to add some additional unit tests for the lambdas.

Will try to run the integration tests as well in my account when I have time - but don't wait for it...

@kichik
Copy link
Member

kichik commented Oct 29, 2023

We forgot to deal with runner deletion. I will have another pass tomorrow to see nothing else was missed.

@kichik kichik changed the title feat: register self-hosted runners on organization level feat: Option to register runners on organization instead of repo Nov 1, 2023
Copy link
Member

@kichik kichik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @pharindoko. Thanks!

@mergify mergify bot merged commit 42c5ade into CloudSnorkel:main Nov 1, 2023
13 checks passed
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.

permissions for repo runner
2 participants