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

Rename Env Keys #6

Open
2 of 6 tasks
Lypsolon opened this issue Jun 21, 2024 · 11 comments
Open
2 of 6 tasks

Rename Env Keys #6

Lypsolon opened this issue Jun 21, 2024 · 11 comments
Assignees

Comments

@Lypsolon
Copy link
Collaborator

Lypsolon commented Jun 21, 2024

Tasks

The current name for the logger settings could be nicer. Let's change them to be a bit more descriptive.

AYONLOGGERLOGLVL = LOG_LEVEL
AYONLOGGERFILELOGGING = ENABLE_FILE_LOGGING
AYONLOGGERFILEPOS = LOG_FILE_PATH

[Blocking PR]

[PRs Waiting on this PR]

@Lypsolon Lypsolon self-assigned this Jun 21, 2024
@Lypsolon
Copy link
Collaborator Author

Lypsolon commented Oct 4, 2024

this issue is currently blocked because of this PR.
ynput/ayon-usd-resolver#36

i would like to merge it before changing the Cpp Api to the new version to void having to deal with potential merge conflicts and to avid having to maintain 2 build setups.

[Edit] its now unblocked.

@Lypsolon
Copy link
Collaborator Author

@antirotor @BigRoy.

The CppApi is now changed to use no Env Vrabiables.
this enables us to change the env keys from the Resolver.

but there are a few Questions about what names we want them to have.

  1. i believe we should keep the AYON_ prefix as LOG_LEVEL dose not inform us what we are controlling
  2. do we want to rename other Env keys while we are on it ?
AYONLOGGERLOGLVL -> AYON_USD_RESOLVER_LOGLVL or AYON_USD_RESOLVER_LOG_LVL
AYONLOGGERFILELOGGING -> AYON_USD_RESOLVER_FILELOGGING 
AYONLOGGERFILEPOS -> AYON_USD_RESOLVER_LOG_FILE
AYON_LOGGIN_LOGGIN_KEYS -> AYON_USD_RESOLVER_LOGGIN_KEYS

ENABLE_STATIC_GLOBAL_CACHE -> AYON_USD_RESOLVER_ENABLE_PINNING
PINNING_FILE_PATH -> AYON_USD_RESOLVER_PINNING_FILE
PROJECT_ROOTS -> AYON_USD_RESOLVER_PROJECT_ROOTS

AYON_API_KEY -> cant be renamed
AYON_SERVER_URL -> cant be renamed
AYON_SITE_ID -> cant be renamed
AYON_PROJECT_NAME -> cant be renamed

TF_DEBUG -> cant be renamed

@Lypsolon
Copy link
Collaborator Author

Also i believe this should be a major version change
or
somehow show in the version its backwards incomparable.

Reason:
if some one has build something ontop of the resolver this will be a braking change.
also the addons should be informed that there is a braking change
(currently only we are working on the addons that consume the Resolver but it might be a good moment to follow the idea of big version ups on backward incompatible changes ?)

@BigRoy
Copy link

BigRoy commented Oct 15, 2024

Also i believe this should be a major version change

Yup - should be fine to do. However, since we're still in experimental I'd personally also allow it to be minor bump still - but it'd go against regular 'version incrementing' rules. @dee-ynput preference?

I'd opt for renaming - and making it clearer what the variables are for. Yes. I'd go with something like:

AYONLOGGERLOGLVL -> AYON_USD_RESOLVER_LOG_LVL
AYONLOGGERFILELOGGING -> AYON_USD_RESOLVER_FILELOGGING 
AYONLOGGERFILEPOS -> AYON_USD_RESOLVER_LOG_FILE
AYON_LOGGIN_LOGGIN_KEYS -> AYON_USD_RESOLVER_LOG_KEYS

ENABLE_STATIC_GLOBAL_CACHE -> AYON_USD_RESOLVER_ENABLE_PINNING
PINNING_FILE_PATH -> AYON_USD_RESOLVER_PINNING_FILE
PROJECT_ROOTS -> AYON_USD_RESOLVER_PROJECT_ROOTS

Why do we need this separately?

AYONLOGGERFILELOGGING -> AYON_USD_RESOLVER_FILELOGGING 
AYONLOGGERFILEPOS -> AYON_USD_RESOLVER_LOG_FILE

Why can't we just assume that when AYON_USD_RESOLVER_LOG_FILE is set that we want file logging enabled?
Preferably we just keep the AYON_USD_RESOLVER_LOG_* keys so they are 'together'.

And for this one:

AYON_USD_RESOLVER_PROJECT_ROOTS

I have the tendency to also change that around so it's related to the pinning. So:

AYON_USD_RESOLVER_PROJECT_ROOTS -> AYON_USD_RESOLVER_PINNING_PROJECT_ROOTS or AYON_USD_RESOLVER_PINNING_ROOTS

@Lypsolon
Copy link
Collaborator Author

Why do we need this separately?

AYONLOGGERFILELOGGING -> AYON_USD_RESOLVER_FILELOGGING 
AYONLOGGERFILEPOS -> AYON_USD_RESOLVER_LOG_FILE

Why can't we just assume that when AYON_USD_RESOLVER_LOG_FILE is set that we want file logging enabled?

we dont do this for developer convenience. this way you can have a file set and disable it. eg for testing or for re running a job without it.
its a handy little feature kinda like the enabled bool on an pyblish instance.

also remember if you set an env variable to be empty on a good few Linux systems the env variable will not be gone.
so you might end up with checking if its removed. (its a rare game but cmake hits me with it every now and then)

Preferably we just keep the AYON_USD_RESOLVER_LOG_* keys so they are 'together'.

agreed.
like this ?

AYONLOGGERLOGLVL -> AYON_USD_RESOLVER_LOG_LVL
AYONLOGGERFILELOGGING -> AYON_USD_RESOLVER_LOG_FILELOGGING 
AYONLOGGERFILEPOS -> AYON_USD_RESOLVER_LOG_FILE
AYON_LOGGIN_LOGGIN_KEYS -> AYON_USD_RESOLVER_LOG_KEYS

ENABLE_STATIC_GLOBAL_CACHE -> AYON_USD_RESOLVER_ENABLE_PINNING
PINNING_FILE_PATH -> AYON_USD_RESOLVER_PINNING_FILE
PROJECT_ROOTS -> AYON_USD_RESOLVER_PINNING_ROOTS

AYON_API_KEY -> cant be renamed
AYON_SERVER_URL -> cant be renamed
AYON_SITE_ID -> cant be renamed
AYON_PROJECT_NAME -> cant be renamed

TF_DEBUG -> cant be renamed

And for this one:

AYON_USD_RESOLVER_PROJECT_ROOTS

I have the tendency to also change that around so it's related to the pinning. So:

AYON_USD_RESOLVER_PROJECT_ROOTS -> AYON_USD_RESOLVER_PINNING_PROJECT_ROOTS or AYON_USD_RESOLVER_PINNING_ROOTS

i like AYON_USD_RESOLVER_PINNING_ROOTS it sounds nice, no other reason tbh ;-)

@BigRoy
Copy link

BigRoy commented Oct 16, 2024

we dont do this for developer convenience. this way you can have a file set and disable it. eg for testing or for re running a job without it.
its a handy little feature kinda like the enabled bool on an pyblish instance.

Fine with me - bit verbose but hey - if there's a use case, fine with me. @antirotor thoughts about having both enabled?

AYON_USD_RESOLVER_FILELOGGING 
AYON_USD_RESOLVER_LOG_FILE

Or maybe a better name is:

AYON_USD_RESOLVER_LOG_FILE_ENABLED
AYON_USD_RESOLVER_LOG_FILE

@Lypsolon
Copy link
Collaborator Author

AYON_USD_RESOLVER_FILELOGGING 
AYON_USD_RESOLVER_LOG_FILE

Or maybe a better name is:

AYON_USD_RESOLVER_LOG_FILE_ENABLED
AYON_USD_RESOLVER_LOG_FILE

i vote for the second option they are more direct and you don't need to guess.

@Lypsolon
Copy link
Collaborator Author

[Status]
this message will give a simple update on the situation.
this task is still blocked by this PR #27
im would like to know who i could assign maybe some one with some c++ experience that would be nice.
in the end of the day everything in the other PR is a implementation question.

@antirotor
Copy link
Member

I vote for the second option too btw.

@dee-ynput
Copy link

#27 is ready to merge, merge it and go on with this one.
Just keep in mind this has a low priority so focus on it once you have availability 👍

@Lypsolon
Copy link
Collaborator Author

the fix for this will be implemented inside ayon usd resolver.
we are now at the point where the cpp api dose not need env variables any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment