Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

Add ENV VAR FLTYE_ADMIN_ENDPOINT (#4948) #466

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zychen5186
Copy link
Contributor

@zychen5186 zychen5186 commented Mar 22, 2024

TL;DR

Add support for assigning admin endpoint by passing config via ENV vars

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

I found that the admin endpoint value is assigned by .yaml configure file via viper, however, the viper accessor function is written in flytestdlib and the repo is already set as read-only, so instead I added a function to override the config if env var is set, after err := configAccessor.UpdateConfig(context.TODO()) in root.go. Also, further need for customized env var could be added in this function as well.

for testing, please set the ENV var $FLTYE_ADMIN_ENDPOINT to the desired url, if ENV var is set, it would be used as the endpoint of admin instead the one specified in the config yaml file. e.g.
export FLTYE_ADMIN_ENDPOINT=localhost:30080

Tracking Issue

flyteorg/flyte#4948

Follow-up issue

NA

cmd/core/cmd.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 52.00000% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 67.59%. Comparing base (848ab01) to head (15debc2).

Files Patch % Lines
cmd/config/env_var_reader.go 59.09% 6 Missing and 3 partials ⚠️
cmd/root.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #466      +/-   ##
==========================================
- Coverage   67.65%   67.59%   -0.06%     
==========================================
  Files         148      149       +1     
  Lines        6643     6668      +25     
==========================================
+ Hits         4494     4507      +13     
- Misses       1859     1868       +9     
- Partials      290      293       +3     
Flag Coverage Δ
unittests 67.59% <52.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/config/env_var_reader.go Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
cmd/config/env_var_reader.go Outdated Show resolved Hide resolved
@EngHabu
Copy link
Contributor

EngHabu commented Apr 3, 2024

All fields that show up in cli are also settable through env vars.
If it's --connection.endpoint them it'll be FLYTE_CONNECTION_ENDPOINT or something like that... You shouldn't need this PR IMO.

@zychen5186
Copy link
Contributor Author

zychen5186 commented Apr 3, 2024

Hi @EngHabu, thanks for pointing this out! I also saw this v.viper.AutomaticEnv() in flytestdlib that I suppose it should be used to bind flags with env var, since the corresponding flag I was trying to assign is --admin.endpoint, I tried both FLYTE_ADMIN_ENDPOINT and ADMIN_ENDPOINT but didn't work.
I found this err := v.bindViperConfigsFromEnv(r) function in flytestdlib which I'm not so sure about, but I guess it might bound --admin.endpoint with ADMIN.ENDPOINT? which is not able to be set as an env var.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants