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

Enable attachment of binary files as charm resources #1147

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nrobinaubertin
Copy link

Description

This enables the attachment of binary files as charm resources.

Fixes: #1000

@jujubot
Copy link
Contributor

jujubot commented Oct 8, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Oct 8, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@@ -2078,7 +2078,7 @@ def _upload(self, data, path, app_name, res_name, res_type, pending_id):

headers['Content-Type'] = 'application/octet-stream'
headers['Content-Length'] = len(data)
headers['Content-Sha384'] = hashlib.sha384(bytes(data, 'utf-8')).hexdigest()
headers['Content-Sha384'] = hashlib.sha384(data).hexdigest()
Copy link
Contributor

Choose a reason for hiding this comment

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

This effectively changes the argument type from str to bytes.

There's one more call site at line 2141 that would have to be updated to match.

P.S. please add an explicit type hint on the argument.

@dimaqq
Copy link
Contributor

dimaqq commented Oct 9, 2024

@nrobinaubertin this PR needs some kind of test.

ideally an integration test, or
a series of steps (this exact charm, this bootstrap, this deploy with this binary artifact)

Copy link

@gfouillet gfouillet left a comment

Choose a reason for hiding this comment

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

Nothing to more than dima.

  • Please add a test that would trigger the bug if not fixed
  • take care of all call site of _upload to ensure data type consistency.

@dimaqq
Copy link
Contributor

dimaqq commented Nov 8, 2024

Niels, I see that the PR where you needed this was closed.

Do you have the bandwidth to update this PR?

If not, I can maybe create an issue based on the PR and eventually someone else can pick this up. What you're fixing is basically a bug, as binary resources should be supported.

@dimaqq
Copy link
Contributor

dimaqq commented Nov 19, 2024

This PR needs to be rebased since #1186 has been merged.

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.

Unable to upload .tar file as local resource to deployed charmed application
4 participants