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: garm runners will be reflected into kubernetes as runner resource #23

Merged
merged 23 commits into from
Dec 11, 2023

Conversation

rafalgalaw
Copy link
Collaborator

@rafalgalaw rafalgalaw commented Nov 1, 2023

with

$ kubectl get runner

one can now list and with

$ kubectl delete runner my-garm-runner

one can delete a runner from within a cluster.
Also, if accidently a runner get deleted via cli from within the garm-server itself, der controller cleans up not matching runner CRs in k8s

Todo:

  • implement tests

@rafalgalaw rafalgalaw marked this pull request as ready for review November 1, 2023 21:37
@rafalgalaw rafalgalaw force-pushed the feat/show-runners branch 6 times, most recently from 986d63d to 7c92935 Compare November 2, 2023 07:29
Copy link
Member

@bavarianbidi bavarianbidi left a comment

Choose a reason for hiding this comment

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

Looks very nice - just few nits

🙏 please squash your commits so we will get nice autogenerated release notes. Otherwise each commit will appear in the release notes as one topic in the feature list

api/v1alpha1/runner_types.go Show resolved Hide resolved
api/v1alpha1/runner_types.go Show resolved Hide resolved
api/v1alpha1/runner_types.go Show resolved Hide resolved
api/v1alpha1/runner_types.go Show resolved Hide resolved
api/v1alpha1/runner_types.go Outdated Show resolved Hide resolved
api/v1alpha1/runner_types.go Show resolved Hide resolved
config/rbac/role.yaml Show resolved Hide resolved
config/rbac/runner_editor_role.yaml Show resolved Hide resolved
internal/controller/runner_controller.go Show resolved Hide resolved
internal/controller/runner_controller.go Outdated Show resolved Hide resolved
Copy link
Member

@bavarianbidi bavarianbidi left a comment

Choose a reason for hiding this comment

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

please update the existing ADR to make the current implementation clear

cmd/main.go Outdated Show resolved Hide resolved
@rafalgalaw rafalgalaw self-assigned this Dec 4, 2023
@rafalgalaw rafalgalaw mentioned this pull request Dec 4, 2023
@rafalgalaw rafalgalaw linked an issue Dec 4, 2023 that may be closed by this pull request
@rafalgalaw
Copy link
Collaborator Author

After communication with @bavarianbidi, I will move the auth refactoring to a separate PR

@rafalgalaw rafalgalaw force-pushed the feat/show-runners branch 2 times, most recently from f2df4ef to 013d6e3 Compare December 6, 2023 11:24
@rafalgalaw rafalgalaw changed the title Feat/show runners feat/show-runners Dec 8, 2023
@rafalgalaw
Copy link
Collaborator Author

rafalgalaw commented Dec 8, 2023

  • make tests work again after refactoring

pkg/client/instance.go Outdated Show resolved Hide resolved

func (r *RunnerReconciler) PollRunnerInstances(ctx context.Context, eventChan chan event.GenericEvent) {
log := log.FromContext(ctx)
ticker := time.NewTicker(5 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

should we have this configurable? maybe it's a nice feature - wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so one could supply a flag in which interval runners are getting polled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I do this in a separate PR, I want to get this feature finally shipped 😅

Copy link
Member

Choose a reason for hiding this comment

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

as discussed, i'm fine with adding an additional flag in a separate PR

rafalgalaw and others added 4 commits December 8, 2023 13:13
Co-authored-by: Mario Constanti <github@constanti.de>
Co-authored-by: Mario Constanti <github@constanti.de>
Co-authored-by: Mario Constanti <github@constanti.de>
Co-authored-by: Mario Constanti <github@constanti.de>
rafalgalaw and others added 4 commits December 8, 2023 15:38
Signed-off-by: Mario Constanti <mario.constanti@mercedes-benz.com>
@bavarianbidi bavarianbidi changed the title feat/show-runners feat: garm runners will be reflected into kubernetes as runner resource Dec 8, 2023
@bavarianbidi bavarianbidi merged commit 94cf503 into main Dec 11, 2023
3 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.

Show Runners
2 participants