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

Reformstudios patch fix shotgun descriptor incorrect system name behaviour #721

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

reformstudios
Copy link
Contributor

Adding a "system_name" field to the "shotgun" descriptor allows for overriding the default behaviour in the "get_system_name" method which returns the wrong name for use in hook templates where the {engine} tag is used.
eg, if a descriptor points to a toolkit bundle names "tk-houdini_release_v1.2.3", and a hook path is of the form "path: /some/path/to/{engine}/hook" then the {engine} field will be replaced by {entity_type}_{entity_id}, which will not point to the expected location of, for example, 'tl-houdini/hook' it will instead point to 'customNonProjectEntity01_104/hook'.
Adding an optional field to allow the user to set the "system_name" manually allows the default behaviour to be overridden where required without breaking existing expected behaviour.
This should resolve the issues raised on the community site on pages :-

Update fork to latest official master
Adding a "system_name" field to the "shotgun" descriptor allows for overriding the default behaviour in the "get_system_name" method which returns the wrong name for use in hook templates where the {engine} tag is used. 
eg, if a descriptor points to a toolkit bundle names "tk-houdini_release_v1.2.3", and a hook path is of the form "path: /some/path/to/{engine}/hook" then the {engine} field will be replaced by {entity_type}_{entity_id}, which will not point to the expected location of, for example, 'tl-houdini/hook' it will instead point to 'customNonProjectEntity01_104/hook'.
Adding an optional field to allow the user to set the "system_name" manually allows the default behaviour to be overridden where required without breaking existing expected behaviour.
This should resolve the issues raised on the community site on pages :- 
- https://community.shotgunsoftware.com/t/possible-hook-path-bug/3365
- https://community.shotgunsoftware.com/t/names-for-apps-frameworks-specified-via-shotgun-descriptors/2989
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3052

  • 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • 188 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-1.05%) to 68.924%

Changes Missing Coverage Covered Lines Changed/Added Lines %
python/tank/descriptor/io_descriptor/shotgun_entity.py 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
python/tank/bootstrap/resolver.py 1 96.52%
python/tank/commands/setup_project_params.py 1 65.6%
python/tank/commands/tank_command.py 1 45.36%
python/tank/util/zip.py 1 94.83%
python/tank/authentication/user_impl.py 2 77.46%
python/tank/authentication/user.py 2 59.34%
python/tank/hook.py 2 89.84%
python/tank/log.py 3 77.3%
python/tank/bootstrap/configuration.py 4 87.04%
python/tank/descriptor/descriptor_config.py 6 88.89%
Totals Coverage Status
Change from base Build 3032: -1.05%
Covered Lines: 12305
Relevant Lines: 17853

💛 - Coveralls

@pscadding pscadding added the Logged logged in Jira label Oct 23, 2019
@reformstudios
Copy link
Contributor Author

@eshokrgozar This bug has hit me again in another studio. Is there any chance this could get reviewed anytime soon? This PR is about 5yrs old now :).

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.75%. Comparing base (7c52d07) to head (eacb388).

Files Patch % Lines
...on/tank/descriptor/io_descriptor/shotgun_entity.py 75.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (7c52d07) and HEAD (eacb388). Click for more details.

HEAD has 12 uploads less than BASE
Flag BASE (7c52d07) HEAD (eacb388)
24 12
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
- Coverage   79.90%   73.75%   -6.15%     
==========================================
  Files         198      198              
  Lines       20719    20722       +3     
==========================================
- Hits        16555    15284    -1271     
- Misses       4164     5438    +1274     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eshokrgozar eshokrgozar removed their assignment Jul 24, 2024
@eshokrgozar
Copy link
Contributor

@eshokrgozar This bug has hit me again in another studio. Is there any chance this could get reviewed anytime soon? This PR is about 5yrs old now :).

Hi Patrick. I don't work at Autodesk anymore. I didn't realize this was still assigned to me and just removed the assignment. Hopefully someone at ADSK can pick it up and help you out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Logged logged in Jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants