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

Resolve environment variable in google drive credential path #3008

Merged
merged 4 commits into from
Apr 7, 2022
Merged

Resolve environment variable in google drive credential path #3008

merged 4 commits into from
Apr 7, 2022

Conversation

ClementHector
Copy link
Contributor

Brief description

Allow resolving a path with an environment variable in gdrive credentials.
For example:
if OpenPype is installed in path/to/OP and setting project_settings/global/sync_server/sites/gdrive/credentials_url is {OPENPYPE_ROOT}/path/to/cred_file.json will give /path/to/OP/path/to/cred_file.json

@iLLiCiTiT
Copy link
Member

I don't think acre merge should be used for this. I understand that part of the logic is same but acre is meant for environment variables and merges all same values together which may be dangerous in this case (calling the acre.merge).

Function which would only parse the dictionary as is and tries to fill env values in, is the way == skip the merge. Something like this:

def prepare_platform_values(data, env=None):
    if env is None:
        env = os.environ
    env_copy = env.copy()
    parsed_data = acre.parse(data)
    for key in tuple(parsed_data.keys()):
        value = parsed_data[key]
        try:
            parsed_data[key] = value.format(**env_copy)
        except KeyError, ValueError:
            pass
   return parsed_data

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 4, 2022

@iLLiCiTiT isn't that just doing acre.prepare and acre.compute? :)

@ClementHector
Copy link
Contributor Author

@iLLiCiTiT isn't that just doing acre.prepare and acre.compute? :)

I ain't find acre.prepare

@ClementHector
Copy link
Contributor Author

I don't think acre merge should be used for this. I understand that part of the logic is same but acre is meant for environment variables and merges all same values together which may be dangerous in this case (calling the acre.merge).

I didn't think it was a problem because the envrionment is copy and not reinject at the end. Using accre made sense to me, I will do a simple format.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Apr 4, 2022

Using accre made sense to me, I will do a simple format.

There is possibility that you'll have environment with name EXAMPLE and will have key stored under EXAMPLE and they'll be merged using os.pathsep which is not expected. In other words the merge does more than should. The formatting part is actually side effect of merge.

isn't that just doing acre.prepare and acre.compute

I would say that compute is for this case more complicated, but yes.

@iLLiCiTiT iLLiCiTiT requested a review from kalisp April 5, 2022 08:07
@iLLiCiTiT iLLiCiTiT added the type: enhancement Enhancements to existing functionality label Apr 5, 2022
@ClementHector ClementHector changed the title Resolve environment variable in credential path with accre Resolve environment variable in google drive credential path Apr 6, 2022
Copy link
Member

@kalisp kalisp left a comment

Choose a reason for hiding this comment

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

Works.

@kalisp kalisp merged commit cdbcfad into ynput:develop Apr 7, 2022
@ClementHector ClementHector deleted the site-sync-gdrive-credential-path-resolve-env branch April 8, 2022 10:13
@mkolar mkolar added this to the next milestone Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Enhancements to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants