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

feat: Backlog/connect config #424

Open
wants to merge 56 commits into
base: main
Choose a base branch
from
Open

Conversation

lluisFtrack
Copy link
Collaborator

Resolves :

  • I have added automatic tests where applicable.
  • The PR contains a description of what has been changed.
  • The description contains manual test instructions.
  • The PR contains update to the release notes.
  • The PR contains update to the documentation.

This PR has been tested on :

  • Windows.
  • MacOs.
  • Linux.

Changes

  • CONNECT_CONFIG_PATH implementation supporting plugin_path and launch_path on the config file.

Test

@lluisFtrack lluisFtrack requested a review from a team as a code owner March 13, 2024 13:23
@lluisFtrack lluisFtrack changed the base branch from main to backlog/connect-utils-cleanup March 13, 2024 13:23
Copy link

github-actions bot commented Mar 13, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

Copy link
Contributor

@henriknorin-ftrack henriknorin-ftrack left a comment

Choose a reason for hiding this comment

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

See feedback

apps/connect/source/ftrack_connect/ui/application.py Outdated Show resolved Hide resolved


def read_json_config():
def read_json_login():
Copy link
Contributor

Choose a reason for hiding this comment

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

Would perhaps name it load_credentials



def write_json_config(config):
def write_json_login(config):
Copy link
Contributor

Choose a reason for hiding this comment

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

store_credentials

apps/connect/source/ftrack_connect/utils/plugin.py Outdated Show resolved Hide resolved
'ftrack_connect', level=loggingLevels[namespace.verbosity]
)

# TODO: Discuss if those keys should might be FTRACK_CONNECT_PLUGIN_PATH and
# FTRACK_CONNECT_LAUNCH_PATH even though they are
# not environment variables anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like as you have it now - plugin_path

@@ -7,15 +7,18 @@
import errno


def get_default_log_directory():
return platformdirs.user_data_dir('ftrack-connect', 'ftrack', 'log')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider moving this into the config file aswell.



def get_config_file_path():
def get_login_file_path():
Copy link
Contributor

Choose a reason for hiding this comment

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

Would perhaps name it get_credentials_file_path


if os.path.isfile(config_file):
logger.debug(u'Reading config from {0}'.format(config_file))
if os.path.isfile(login_file):
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it does not port over the existing config.json, I think we need to do this so all users do not need to login to Connect again.

config_file = os.path.join(
platformdirs.user_data_dir('ftrack-connect', 'ftrack'), 'config.json'
login_file = os.path.join(
platformdirs.user_data_dir('ftrack-connect', 'ftrack'), 'login.json'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
platformdirs.user_data_dir('ftrack-connect', 'ftrack'), 'login.json'
platformdirs.user_data_dir('ftrack-connect', 'ftrack'), 'credentials.json'

@henriknorin-ftrack
Copy link
Contributor

Idéa to make migration easier: if FTRACK_CONNECT_PLUGIN_PATH is detected, output a deprecation warning to logs and tell users to achieve this through the config file instead.

Base automatically changed from backlog/connect-utils-cleanup to main March 20, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants