flytectl icon indicating copy to clipboard operation
flytectl copied to clipboard

Add ENV VAR FLTYE_ADMIN_ENDPOINT (#4948)

Open zychen5186 opened this issue 1 year ago • 3 comments

TL;DR

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

Type

  • [ ] Bug Fix
  • [x] Feature
  • [ ] Plugin

Are all requirements met?

  • [x] Code completed
  • [x] Smoke tested
  • [x] 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

https://github.com/flyteorg/flyte/issues/4948

Follow-up issue

NA

zychen5186 avatar Mar 22 '24 05:03 zychen5186

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 :warning:
cmd/root.go 0.00% 3 Missing :warning:
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%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 25 '24 18:03 codecov[bot]

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.

EngHabu avatar Apr 03 '24 06:04 EngHabu

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.

zychen5186 avatar Apr 03 '24 07:04 zychen5186