https://flyte.org logo
Powered by
#flyte-deployment
Title
# flyte-deployment
m

Mike Ossareh

03/27/2023, 5:15 PM
When using the 
flyte-binary
helm chart is there a way to specify which project's you'd like automatically defined? I'm looking for something like 
initialProjects
from the 
flyte-core
helm chart
I'm also happy to contribute the change to the 
flyte-binary
helm chart which calls 
flyteadmin
in an 
initContainer
, but I'm working on the basis this wasn't ported to 
flyte-binary
for a reason?
s

Samhita Alla

03/28/2023, 9:41 AM
cc @Yee /@jeev
j

jeev

03/28/2023, 10:50 AM
y

Yee

03/28/2023, 4:15 PM
it wasn’t ported because i think it was an extra init container in the other chart, and we just wanted to avoid that.
j

jeev

03/28/2023, 4:16 PM
maybe we can just make it a config?
y

Yee

03/28/2023, 4:17 PM
we can i suppose.
adds to the config though, and it’s a lot as it is.
j

jeev

03/28/2023, 4:17 PM
true
y

Yee

03/28/2023, 4:17 PM
not against it, just a bit concerned about config bloat
m

Mike Ossareh

03/28/2023, 4:18 PM
y

Yee

03/28/2023, 4:18 PM
helm config
m

Mike Ossareh

03/28/2023, 4:18 PM
got it 👍🏼
y

Yee

03/28/2023, 4:19 PM
flyte admin config is more easily managed i feel
if we add this, i’d add this only to the single binary config
m

Mike Ossareh

03/28/2023, 4:19 PM
From my perspective, being able to update our helm values.yaml and pass values into the admin_config has been quite nice.
j

jeev

03/28/2023, 4:19 PM
right
y

Yee

03/28/2023, 4:20 PM
like the cfg.Admin block here
j

jeev

03/28/2023, 4:20 PM
y

Yee

03/28/2023, 4:20 PM
yeah right?
j

jeev

03/28/2023, 4:21 PM
that’s what i was thinking too. then some plumbing from helm values into the configmap?
y

Yee

03/28/2023, 4:21 PM
yeah
mike you want to do this? or jeev?
j

jeev

03/28/2023, 4:21 PM
@Mike Ossareh up to you
m

Mike Ossareh

03/28/2023, 4:22 PM
I'm happy to take a look at it. It gives me a good reason to better familiarize myself with your configmap.
y

Yee

03/28/2023, 4:22 PM
awesome
🙂
m

Mike Ossareh

03/28/2023, 4:22 PM
is flytestdlib/config just a light wrapper over pflag?
(lazy web, I'm looking now :))
y

Yee

03/28/2023, 4:23 PM
yeah
m

Mike Ossareh

03/28/2023, 4:25 PM
If I create an issue to track this; can one of you assign it to me?
y

Yee

03/28/2023, 4:25 PM
yup
thanks
m

Mike Ossareh

03/28/2023, 4:25 PM
👍🏼
ty
y

Yee

03/28/2023, 4:34 PM
thank you
m

Mike Ossareh

05/02/2023, 7:09 PM
@jeev & @Yee I (finally!!) got around to implementing this. https://github.com/flyteorg/flyte/pull/3631 I'm not sure where is best to document this change; I'm happy to do so, but I'll need a pointer to where best to put those docs.
y

Yee

05/02/2023, 11:05 PM
oh nice thanks.
not to pile, but would you be up for adding to the flyte-binary helm chart as well?
i think it’s okay to keep as seed projects
j

jeev

05/08/2023, 3:02 PM
@Mike Ossareh looks like the PR is close and just needs some final touches :)
wanna talk about the names and configmap structure here?
m

Mike Ossareh

05/08/2023, 3:26 PM
hey @jeev 👋🏼
just looking over the changes; there are three outstanding: • not bumping the chart number - I’ve rolled this back • moving the flyte args up to 000-core.yaml - done • what to call the helm chart key - I’m happy to take direction on this.
j

jeev

05/08/2023, 3:50 PM
@Mike Ossareh: still seeing the version changes, but that probably needs 
make helm
as far as the helm chart key goes: do we even need it? or can we just expose directly under the 
configuration
key? im not super opinionated on this. will defer to you and @Yee. everything else lgtm! thanks for the valuable contribution!
m

Mike Ossareh

05/08/2023, 4:10 PM
@jeev good spot with the `make helm`; I forgot that. I’ve run it and the version changes have bumped. re: the configuration key. You’re suggesting: • 
configuration.admin
• 
configuration.propeller
• 
configuration.dataCatalog
instead of 
configuration.flyte-binary.[admin,propeller,dataCatalog]
. I’m fine with this personally; it’s sort of the same thing from my POV 🙂 so I’m happy to do whatever you and @Yee suggest I do.
j

jeev

05/08/2023, 4:35 PM
no strong opinions from me. i was suggesting that because all of configuration is technically already for 
flyte-binary
m

Mike Ossareh

05/09/2023, 3:11 AM
@jeev / @Yee here’s a diff that demonstrates the suggestion above: https://github.com/ossareh/flyte/pull/1/files some things to consider: 1. 
000-core.yaml
already has a top level 
admin
entry 2. the args for 
flyte-binary
are in a 
Section
called 
flyte
a. we either change 
cmd/single/config.go
to use a different section name b. or we accept that 000-core.yaml cannot have a top level 
admin
key and we put the flyte-binary args under 
flyte:
in the ConfgMap. If the diff above is acceptable, I’ll cherry pick the commit onto the existing PR in the flyte repo - the PR above is just for demonstration purposes.
Here’s another example: https://github.com/ossareh/flyte/pull/2 This is based on this feedback from Yee.
y

Yee

05/09/2023, 4:50 PM
i like that last one.
jeev if you don’t mind can we just merge that?
j

jeev

05/09/2023, 5:04 PM
yea feel free!
m

Mike Ossareh

05/09/2023, 6:05 PM
OK; I'll migrate the commit over to the main PR branch and then ping y'all here when it's pushed.
@Yee I've pushed the update with the new values.yaml - as per above. LMK if you see anything odd.
6 Views
Powered by