Skip to content

Conversation

@zaro0508
Copy link
Contributor

@zaro0508 zaro0508 commented Sep 17, 2025

Refactor get_synapse_owner_id method to get the owner ID from both synapse::ownerId and aws:servicecatalog:provisioningPrincipalArn tags with the former as the preferred option and the latter as the fallback option.

This implements the following proposal in IT-4483:

For a SC product Updates action the lambda currently also gets the synapse id
from the aws:servicecatalog:provisioningPrincipalArn tag.  We would need to
refactor the lambda to get the synapse id from thesynapse:owerId tag instead.
Then the lambda will generate and apply all of the synapse prefixed tags with the
new owner info. 

Refactor get_synapse_owner_id method to get the owner ID from both
synapse::ownerId and aws:servicecatalog:provisioningPrincipalArn
tags with the former as the preffered option and the latter as the
fallback option.
@zaro0508 zaro0508 requested review from a team as code owners September 17, 2025 21:09
@zaro0508 zaro0508 requested review from Copilot and removed request for a team September 17, 2025 21:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactor the get_synapse_owner_id method to prioritize the synapse:ownerId tag over the aws:servicecatalog:provisioningPrincipalArn tag, providing a fallback mechanism for owner identification.

  • Updated function logic to check for synapse:ownerId first, then fall back to extracting ID from the provisioning principal ARN
  • Changed error handling from raising ValueError to returning None when no valid tags are found
  • Enhanced input validation with proper TypeError for unsupported input types

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
set_tags/utils.py Refactored get_synapse_owner_id function to implement tag priority logic and improved error handling
tests/unit/utils/test_get_synapse_owner_id.py Comprehensive test suite rewrite covering all scenarios including both tag types, priority behavior, and edge cases

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@zaro0508 zaro0508 requested review from a team and Copilot September 17, 2025 21:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@zaro0508 zaro0508 merged commit 0ea6b90 into Sage-Bionetworks-IT:master Sep 25, 2025
5 checks passed
zaro0508 added a commit that referenced this pull request Sep 26, 2025
Add a script to allow admins to easily change supported service catalog provisioned product owners.

This script will do the following:
1. Change owner at the service catalog product level
2. Update provisioned resource tags with the new owner info
3. Update bucket policies to allow the new owner read/write access to the bucket

Limitations:
1.  New users are able to stop, start, and restart the instance from the service catalog actions.  However they will not be able to access an EC2 terminal session or notebook or webui session. 

Note: this script only works when executed with Admin access

depends on #63
zaro0508 added a commit to zaro0508/organizations-infra that referenced this pull request Sep 26, 2025
Update teh cfn-cr-synapse-tagger lambda to include a refactored
get_synapse_owner_id method.

depends on Sage-Bionetworks-IT/cfn-cr-synapse-tagger#63
zaro0508 added a commit to Sage-Bionetworks-IT/organizations-infra that referenced this pull request Sep 29, 2025
Update teh cfn-cr-synapse-tagger lambda to include a refactored
get_synapse_owner_id method.

depends on Sage-Bionetworks-IT/cfn-cr-synapse-tagger#63
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