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(pat-3825): discover dpm cloud token from session.json - Python #103

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

gunnerpeterson
Copy link
Contributor

Tested by printing results from getDpmAuthToken with sample tech.patch.dpm/session.json directory and no environment variable.

@linear
Copy link

linear bot commented Aug 8, 2023

Copy link
Contributor

@ajmasci ajmasci left a comment

Choose a reason for hiding this comment

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

A few minor fixes requested, but LGTM

static/python/src/backends/factory.py Outdated Show resolved Hide resolved
session_dir = os.path.join(root_dir, '.config', 'dpm', 'session.json')

try:
with open(session_dir, 'r') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If none of the platforms match (unlikely), then session_dir is undefined. So, define session_path = '' before the if block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered if there's a way we can declare (probably in Cargo.toml) that dpm does not support anything other than some set of target triples that we provide. Alas, that doesn't seem to exist:

Agree that this is a nit. In the docs leading up to the cargo install dpm we'll be clear that we only support Windows, macOS, and Linux.

session_dir = os.path.join(root_dir, '.config', 'dpm', 'session.json')

try:
with open(session_dir, 'r') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered if there's a way we can declare (probably in Cargo.toml) that dpm does not support anything other than some set of target triples that we provide. Alas, that doesn't seem to exist:

Agree that this is a nit. In the docs leading up to the cargo install dpm we'll be clear that we only support Windows, macOS, and Linux.

@gunnerpeterson gunnerpeterson merged commit f96789a into main Aug 8, 2023
10 checks passed
@gunnerpeterson gunnerpeterson deleted the pat-3825-cloud-token-py branch August 8, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants