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

Header args #221

Merged
merged 12 commits into from
Nov 23, 2023
Merged

Header args #221

merged 12 commits into from
Nov 23, 2023

Conversation

vbaltrusaitis-reef
Copy link

Adds --expires, --content-disposition, --content-encoding, --content-language options to subcommands upload-file, upload-unbound-stream, copy-file-by-id.

It used as-of-yet unreleased b2sdk version, hence the draft PR.

@mpnowacki-reef
Copy link

before reading into the thing: sync does not support these flags by design?

@@ -1053,6 +1053,31 @@ def _setup_parser(cls, parser):
parser.add_argument('--metadataDirective', default=None, help=argparse.SUPPRESS)
parser.add_argument('--contentType')
parser.add_argument('--range', type=parse_range)
parser.add_argument(

Choose a reason for hiding this comment

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

maybe this requirement has to discussed again, but we wanted passing both these arguments and their --info b2- counterparts should yield a warning, making it clear which value is used (the direct, non --info one) and even though sdk does logger.warn in that case, I believe this warning will not propagate to CLI users unless they use non default logging settings. Besides, I think CLI should not rely on that happening, and explicit check will not go to waste, and the warning will be replaced by an error in v4. let me know what you think.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you're right, it should emit a warning in CLI if info in file_info and explicit args differs. Will add.

I do have a case to make against making this an error or forcing to use explicit arguments instead of file_info: these pieces of information as part of file_info dictionary is pretty deeply embedded into B2 - it's in the UI, it's in the API, and here the CLI also actually returns the file_info as returned by B2. If someone does some bash processing with jq, it seems pretty reasonable to, say, just copy the file_info from some result and pass that with file_info, expecting it to work.

Choose a reason for hiding this comment

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

I see your points, let's discuss this, this is important stuff

help=
"optional Content-Language header, value based on RFC 2616 section 14.12, example: 'mi, en'"
)
parser.add_argument(
Copy link

@mpnowacki-reef mpnowacki-reef Nov 19, 2023

Choose a reason for hiding this comment

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

I know it's just 5 arguments, but maybe their declaration should be made common between this command and line 2923?

@vbaltrusaitis-reef
Copy link
Author

vbaltrusaitis-reef commented Nov 19, 2023

@mpnowacki-reef

before reading into the thing: sync does not support these flags by design?

More like "sync wasn't mentioned in the ticket or the GitHub issue and I didn't think of it". But use-case and design for these flags for sync is definitely not as clear as for other commands:

  1. Only some files will actually be updated when doing sync (I think?). Unless we significantly significantly change how sync is implemented, using a flag would not provide guarantees that all files will have that thing set. This is a piece behavior that can be surprising.
  2. I may be wrong but setting, say, singular --content-language for all synced files seems a rather niche use-case. Perhaps a different design needs to be explored: setting these properties for subsets of files, or mass-assigning these properties to lists of files.
  3. Analogous flags weren't implemented for sync (e.g. --contentType, --file-info).

I'm not saying that sync shouldn't have this, just that if and how is non-obvious to me.

@mpnowacki-reef
Copy link

mpnowacki-reef commented Nov 19, 2023

@mpnowacki-reef

before reading into the thing: sync does not support these flags by design?

More like "sync wasn't mentioned in the ticket or the GitHub issue and I didn't think of it". But use-case and design for these flags for sync is definitely not as clear as for other commands:

  1. Only some files will actually be updated when doing sync (I think?). Unless we significantly significantly change how sync is implemented, using a flag would not provide guarantees that all files will have that thing set. This is a piece behavior that can be surprising.
  2. I may be wrong but setting, say, singular --content-language for all synced files seems a rather niche use-case. Perhaps a different design needs to be explored: setting these properties for subsets of files, or mass-assigning these properties to lists of files.
  3. Analogous flags weren't implemented for sync (e.g. --contentType, --file-info).

I'm not saying that sync shouldn't have this, just that if and how is non-obvious to me.

maybe let's leave it for a separate PR then, if at all. This definitely warrants a disccusion, as the new cp command is gonna come. A use case I have in mind is: you're using a public bucket as a source for static files, and on each build you want to upload them, preferably setting cache-control (maybe disposition and language as well, who knows).

if file_info is not None and key in file_info and file_info[key] != value:
overwritten.append(key)

if overwritten:

Choose a reason for hiding this comment

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

this is great stuff!

@mpnowacki-reef
Copy link

LGTM, ok to merge after adapting to new CHANGELOG flow

@mpnowacki-reef mpnowacki-reef merged commit 93ce517 into master Nov 23, 2023
23 of 27 checks passed
@mpnowacki-reef mpnowacki-reef deleted the header-args-v2 branch November 23, 2023 20:30
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