Skip to content

many: container validation improvements#13682

Merged
Meulengracht merged 1 commit into
canonical:masterfrom
ZeyadYasser:refactor-container-validation
Mar 14, 2024
Merged

many: container validation improvements#13682
Meulengracht merged 1 commit into
canonical:masterfrom
ZeyadYasser:refactor-container-validation

Conversation

@ZeyadYasser

Copy link
Copy Markdown
Contributor

No description provided.

@ZeyadYasser ZeyadYasser added this to the 2.62 milestone Mar 11, 2024
@ZeyadYasser ZeyadYasser requested review from ernestl and pedronis March 11, 2024 08:06
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 92.10526% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 78.90%. Comparing base (29a6e75) to head (7be84b7).
Report is 10 commits behind head on master.

Files Patch % Lines
snap/container.go 92.30% 4 Missing and 2 partials ⚠️
snap/squashfs/squashfs.go 90.32% 2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13682      +/-   ##
==========================================
+ Coverage   78.87%   78.90%   +0.02%     
==========================================
  Files        1039     1040       +1     
  Lines      133603   133966     +363     
==========================================
+ Hits       105381   105704     +323     
- Misses      21637    21665      +28     
- Partials     6585     6597      +12     
Flag Coverage Δ
unittests 78.90% <92.10%> (+0.02%) ⬆️

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

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

@pedronis pedronis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks

@ernestl ernestl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

@alexmurray

Copy link
Copy Markdown
Contributor

LGTM!

@zyga zyga left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't read test code but I have some comments on the evaluation details. Have a look please.

Comment thread snap/container.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This limit should be documented somewhere. It would be good if there was a matching kernel-side limit so that our limit is not surprising to people.

Comment thread snap/container.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use the long form of for here?

Comment thread snap/container.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is somewhat costly for what is requested. Is this somehow different than strings.HasPrefix or am I missing something subtle?

Comment thread snap/container.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this strings.HasPrefix?

Comment thread snap/container.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this strings.HasPrefix again?

Comment thread snap/container.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you clarify what is innocent here?

Comment thread snap/container.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add a context here, like "Cannot evaluate and validate symbolic link: %s"

@ZeyadYasser ZeyadYasser force-pushed the refactor-container-validation branch from 7be84b7 to 358ca95 Compare March 14, 2024 08:53
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
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.

7 participants