Skip to content

[SYNPY-893] Add multiple profile support #1194

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

Merged
merged 39 commits into from
Apr 29, 2025
Merged

Conversation

BryanFauble
Copy link
Member

This PR is a copy of #1183 - But made from within a non-forked repo/branch.

carmmmm and others added 30 commits March 16, 2025 14:02
…ly passes authentication parameters and lets credential_provider_chain.get_credentials() handle authentication order.
…on to avoid repetition and improve clarity for users.
- Added missing assertions to verify that  and  are called or not as expected in various test cases.
- Refactored tests to ensure proper behavior when profile-related logic is skipped, such as when an auth token is provided.
-Implement profile mismatch warning and logging in the login process
Tracking and displaying authentication profile patches
@BryanFauble BryanFauble mentioned this pull request Apr 22, 2025
3 tasks
@BryanFauble BryanFauble marked this pull request as ready for review April 22, 2025 23:00
@BryanFauble BryanFauble requested a review from a team as a code owner April 22, 2025 23:00
BryanFauble added a commit that referenced this pull request Apr 22, 2025
* Add multiple profile support

---------

Co-authored-by: Carmen Montero <[email protected]>
Co-authored-by: BryanFauble <[email protected]>
@BryanFauble
Copy link
Member Author

@carmmmm There are a few patches needed for some integration tests that failed.

The run logs can be found here:
https://github.com/Sage-Bionetworks/synapsePythonClient/actions/runs/14605880964/job/40974698395#step:10:21154

For reference, the dev server settings used for integration testing are described here:
https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/CONTRIBUTING.md#integration-testing-against-the-dev-synapse-server

@thomasyu888 @xschildw I'd like @carmmmm to be able to use the dev server to run integration tests during local development. Could we get an account created? Do we have a policy regarding test account access for external collaborators?

@carmmmm
Copy link
Contributor

carmmmm commented Apr 23, 2025

@carmmmm There are a few patches needed for some integration tests that failed.

The run logs can be found here: https://github.com/Sage-Bionetworks/synapsePythonClient/actions/runs/14605880964/job/40974698395#step:10:21154

For reference, the dev server settings used for integration testing are described here: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/CONTRIBUTING.md#integration-testing-against-the-dev-synapse-server

@thomasyu888 @xschildw I'd like @carmmmm to be able to use the dev server to run integration tests during local development. Could we get an account created? Do we have a policy regarding test account access for external collaborators?

Thanks for the heads up, and I really appreciate you looking into this! Happy to patch whatever’s needed. I’ll take a look at the logs and follow up with any questions.

And thank you also for looping in folks about dev server access, that’d be super helpful!

@BryanFauble
Copy link
Member Author

Thanks for the heads up, and I really appreciate you looking into this! Happy to patch whatever’s needed. I’ll take a look at the logs and follow up with any questions.

And thank you also for looping in folks about dev server access, that’d be super helpful!

@carmmmm I submitted a request to create a dev account. In the mean time staging might be able to be used (Using the same access token as prod). The .synapseConfig file has these updated endpoints:

[endpoints]
repoEndpoint=https://repo-staging.prod.sagebase.org/repo/v1
authEndpoint=https://repo-staging.prod.sagebase.org/auth/v1
fileHandleEndpoint=https://repo-staging.prod.sagebase.org/file/v1

When you get a chance see if you can use staging. Please create a pull request into this feature branch from your fork with any patches for the tests and I can get it merged in.

@carmmmm
Copy link
Contributor

carmmmm commented Apr 23, 2025

Thanks for the heads up, and I really appreciate you looking into this! Happy to patch whatever’s needed. I’ll take a look at the logs and follow up with any questions.
And thank you also for looping in folks about dev server access, that’d be super helpful!

@carmmmm I submitted a request to create a dev account. In the mean time staging might be able to be used (Using the same access token as prod). The .synapseConfig file has these updated endpoints:

[endpoints]
repoEndpoint=https://repo-staging.prod.sagebase.org/repo/v1
authEndpoint=https://repo-staging.prod.sagebase.org/auth/v1
fileHandleEndpoint=https://repo-staging.prod.sagebase.org/file/v1

When you get a chance see if you can use staging. Please create a pull request into this feature branch from your fork with any patches for the tests and I can get it merged in.

I completed the Synapse Certified User quiz and confirmed via getUserProfile() that I’m certified and logged in as my username. However, when running the test_command_line_client test suite against staging (even after waiting an hour for it to update), I’m still getting a 403: Only certified users may create or update content in Synapse.

Is there have a separate staging token, or is staging supposed to accept the prod token?

I know there’s an option to patch the endpoint programmatically for tests, but that feels a bit messy unless we know for sure a staging or dev token isn’t coming. Just wanted to check what you’d recommend before I go further!

@BryanFauble
Copy link
Member Author

Thanks for the heads up, and I really appreciate you looking into this! Happy to patch whatever’s needed. I’ll take a look at the logs and follow up with any questions.
And thank you also for looping in folks about dev server access, that’d be super helpful!

@carmmmm I submitted a request to create a dev account. In the mean time staging might be able to be used (Using the same access token as prod). The .synapseConfig file has these updated endpoints:

[endpoints]
repoEndpoint=https://repo-staging.prod.sagebase.org/repo/v1
authEndpoint=https://repo-staging.prod.sagebase.org/auth/v1
fileHandleEndpoint=https://repo-staging.prod.sagebase.org/file/v1

When you get a chance see if you can use staging. Please create a pull request into this feature branch from your fork with any patches for the tests and I can get it merged in.

I completed the Synapse Certified User quiz and confirmed via getUserProfile() that I’m certified and logged in as my username. However, when running the test_command_line_client test suite against staging (even after waiting an hour for it to update), I’m still getting a 403: Only certified users may create or update content in Synapse.

Is there have a separate staging token, or is staging supposed to accept the prod token?

I know there’s an option to patch the endpoint programmatically for tests, but that feels a bit messy unless we know for sure a staging or dev token isn’t coming. Just wanted to check what you’d recommend before I go further!

Thanks for trying this. The data in staging is a copy of production data, but that copy process occurs at some cadence (I want to say weekly?). This is going to force you to try running these tests against production until the next copy of data makes it to staging (Or we get you the dev account).

I would recommend that you run only the affected tests, and empty your trash bin after your done testing. The tests create a bunch of resources to run the test against, but everything should get cleaned up and marked as deleted.

Is there have a separate staging token, or is staging supposed to accept the prod token?

It should work with the same token.

@carmmmm
Copy link
Contributor

carmmmm commented Apr 23, 2025

Thanks for the heads up, and I really appreciate you looking into this! Happy to patch whatever’s needed. I’ll take a look at the logs and follow up with any questions.
And thank you also for looping in folks about dev server access, that’d be super helpful!

@carmmmm I submitted a request to create a dev account. In the mean time staging might be able to be used (Using the same access token as prod). The .synapseConfig file has these updated endpoints:

[endpoints]
repoEndpoint=https://repo-staging.prod.sagebase.org/repo/v1
authEndpoint=https://repo-staging.prod.sagebase.org/auth/v1
fileHandleEndpoint=https://repo-staging.prod.sagebase.org/file/v1

When you get a chance see if you can use staging. Please create a pull request into this feature branch from your fork with any patches for the tests and I can get it merged in.

I completed the Synapse Certified User quiz and confirmed via getUserProfile() that I’m certified and logged in as my username. However, when running the test_command_line_client test suite against staging (even after waiting an hour for it to update), I’m still getting a 403: Only certified users may create or update content in Synapse.
Is there have a separate staging token, or is staging supposed to accept the prod token?
I know there’s an option to patch the endpoint programmatically for tests, but that feels a bit messy unless we know for sure a staging or dev token isn’t coming. Just wanted to check what you’d recommend before I go further!

Thanks for trying this. The data in staging is a copy of production data, but that copy process occurs at some cadence (I want to say weekly?). This is going to force you to try running these tests against production until the next copy of data makes it to staging (Or we get you the dev account).

I would recommend that you run only the affected tests, and empty your trash bin after your done testing. The tests create a bunch of resources to run the test against, but everything should get cleaned up and marked as deleted.

Is there have a separate staging token, or is staging supposed to accept the prod token?

It should work with the same token.

Got it, thanks! that makes sense. I’ll go ahead and run just the affected tests against prod and clean out my trash bin after. I’ll keep an eye out for the dev account or staging sync update too.

@carmmmm
Copy link
Contributor

carmmmm commented Apr 25, 2025

Please create a pull request into this feature branch from your fork with any patches for the tests and I can get it merged in.

Hey @BryanFauble — I think I did what you meant here by opening this PR into the feature branch with the patches for the tests (let me know if it’s not where you expected it to go!) https://github.com/BryanFauble/synapsePythonClientMultipleProfiles/pull/1

@BryanFauble
Copy link
Member Author

Please create a pull request into this feature branch from your fork with any patches for the tests and I can get it merged in.

Hey @BryanFauble — I think I did what you meant here by opening this PR into the feature branch with the patches for the tests (let me know if it’s not where you expected it to go!) BryanFauble#1

Thanks @carmmmm , It's almost what I had meant. Are you able to create a pull request that points at the main (Non-forked) repo for this branch: https://github.com/Sage-Bionetworks/synapsePythonClient/tree/add-multiple-profiles

@carmmmm
Copy link
Contributor

carmmmm commented Apr 25, 2025

@BryanFauble Sorry! I tried again, is this what you meant? #1198

@BryanFauble
Copy link
Member Author

@BryanFauble Sorry! I tried again, is this what you meant? #1198

@carmmmm Exactly! It looks like just 1 issue to patch up with this merge conflict, and I can merge the change back in here so we can verify the integration/unit test run works!

image

@BryanFauble BryanFauble merged commit 5b0d7fe into develop Apr 29, 2025
28 checks passed
@BryanFauble BryanFauble deleted the add-multiple-profiles branch April 29, 2025 23:05
BryanFauble added a commit that referenced this pull request Apr 29, 2025
BryanFauble added a commit that referenced this pull request Apr 29, 2025
* Add multiple profile support to the Synapse config file, allowing a single config file to log into several different synapse accounts

---------

Co-authored-by: Carmen Montero <[email protected]>
Co-authored-by: BryanFauble <[email protected]>
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