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

Adds option to specify workPool #121

Merged
merged 5 commits into from
Mar 13, 2023
Merged

Adds option to specify workPool #121

merged 5 commits into from
Mar 13, 2023

Conversation

EmilRex
Copy link
Contributor

@EmilRex EmilRex commented Feb 27, 2023

Adds option to set a single workPool in chart.

Still need to test the change to verify that existing behavior isn't changed and new behavior is.

@EmilRex EmilRex requested review from gabcoyne, zanieb and a team as code owners February 27, 2023 18:19
jamiezieziula
jamiezieziula previously approved these changes Feb 27, 2023
Copy link
Contributor

@jamiezieziula jamiezieziula left a comment

Choose a reason for hiding this comment

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

lgtm

parkedwards
parkedwards previously approved these changes Feb 27, 2023
@EmilRex EmilRex changed the title Adds option to specify workPool [DRAFT] Adds option to specify workPool Feb 28, 2023
@EmilRex EmilRex changed the title [DRAFT] Adds option to specify workPool Adds option to specify workPool Feb 28, 2023
jimid27
jimid27 previously approved these changes Mar 1, 2023
Copy link
Contributor

@jimid27 jimid27 left a comment

Choose a reason for hiding this comment

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

nice looks good

@jimid27 jimid27 dismissed stale reviews from parkedwards, jamiezieziula, and themself via 9f2f1e3 March 1, 2023 23:13
@jicuss
Copy link

jicuss commented Mar 3, 2023

this would be super helpful to me

@jsopkin
Copy link
Contributor

jsopkin commented Mar 6, 2023

Hey y'all, I tested out the chart locally. I pulled down Emil's chart and deployed the agent on minikube. I ended up hardcoding the work pool for the sake of testing, but it was able to deploy the agent successfully. @EmilRex If you can sync sometime I can show you how I tested the chart

@EmilRex EmilRex requested review from jimid27, parkedwards and jamiezieziula and removed request for jimid27 and parkedwards March 10, 2023 15:19
@EmilRex
Copy link
Contributor Author

EmilRex commented Mar 10, 2023

Alright... I made (and tested) some changes to the default behavior. Now if neither workPool nor workQueues are specified, we just listen to the default work queue. This ensures that the default behavior of the chart remains unchanged. Then, if either (or both) workPool or workQueues is specified, we fill them in as appropriate.

@jimid27 Does that make sense to you?

@EmilRex EmilRex requested review from jimid27 and removed request for jamiezieziula March 13, 2023 15:50
Copy link
Contributor

@jimid27 jimid27 left a comment

Choose a reason for hiding this comment

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

This lgtm!

@gabcoyne gabcoyne merged commit 63893b7 into PrefectHQ:main Mar 13, 2023
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.

7 participants