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

🐛 Project copy failing when pennsieve token is active #3509

Merged

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Nov 3, 2022

What do these changes do?

This PR changes the call to compute the project size.
BEFORE: would check every storage location to get the total size of the project
AFTER: only computes the project total size reported by the internal Simcore S3 storage location

Rationale: in the way this is currently implemented we cannot ask the Pennsieve to give us this information (missing API, and others)

This problem prevents copying any project or opening templates.

@pcrespov this will need to be hotfixed to production

Related issue/s

relates to ITISFoundation/osparc-issues#644

How to test

Checklist

Copy link
Member

@odeimaiz odeimaiz left a comment

Choose a reason for hiding this comment

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

merci 👍

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #3509 (3701c9d) into master (3701c9d) will not change coverage.
The diff coverage is n/a.

❗ Current head 3701c9d differs from pull request most recent head 5e2384c. Consider uploading reports for the commit 5e2384c to get more accurate results

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3509   +/-   ##
======================================
  Coverage    81.0%   81.0%           
======================================
  Files         834     834           
  Lines       35459   35459           
  Branches      748     748           
======================================
  Hits        28741   28741           
  Misses       6528    6528           
  Partials      190     190           
Flag Coverage Δ
integrationtests 67.5% <0.0%> (ø)
unittests 77.8% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

for file_metadata in list_of_files_enveloped.data:
project_size_bytes += file_metadata.file_size
project_size = parse_obj_as(ByteSize, project_size_bytes)
log.info(
Copy link
Member

Choose a reason for hiding this comment

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

MINOR: i know this was there before but just as a comment. I do not see the point of logging.INFO a return value. It should be the caller who does that (if needed) since he has both the project_uuid (input) and project_zsiez (output).
For the sake of discussion: imaging we call this function in a for loop to find some specific size ... you get an unwished info log.

Copy link
Member Author

Choose a reason for hiding this comment

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

we are anyway not logging info logs

Copy link
Member Author

Choose a reason for hiding this comment

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

I remove it. But then I can ask for a lot of the logs... imagine we have 10000 users calling into the handlers...

Copy link
Member

@pcrespov pcrespov Nov 4, 2022

Choose a reason for hiding this comment

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

I do not mean that you should not put logs. I mean that IMO it should be placed outside this function i.e. by the caller :-)

@sanderegg sanderegg force-pushed the bugfix/datcore_does_not_filter branch from 3f4bfc7 to 5e2384c Compare November 4, 2022 09:28
@sonarcloud
Copy link

sonarcloud bot commented Nov 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sanderegg sanderegg merged commit 058aadc into ITISFoundation:master Nov 4, 2022
@sanderegg sanderegg deleted the bugfix/datcore_does_not_filter branch November 4, 2022 10:20
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.

None yet

4 participants