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 generation refactoring #2436

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

Header generation refactoring #2436

wants to merge 58 commits into from

Conversation

ATorrise
Copy link
Contributor

@ATorrise ATorrise commented Feb 14, 2025

This refactoring effort centralizes and standardizes header generation across the SDK and CLI, consolidating all header logic into a single utility class—ZosFilesHeaders—and replacing duplicate header creation code across multiple methods.

The new implementation leverages a unified headerMap to manage header generation functions, ensuring that headers like responseTimeout, etag, and others are applied consistently in all ZosFiles REST calls.

  • Header Generation now:

    • depends on:
      • the request options
    • optionally takes:
      • dataLength parameter to add a Content-Length header when needed.
      • An enum (ZosFilesContext) that guides which Content-Type headers are applied for certain methods. The enum values include::
        • USS_MULTIPLE: Forces a general content-type header ("Content-Type": "application/json") when multiple USS files.
        • ZFS& LIST: No content-type headers are applied.
        • UPLOAD: Applies the content-type header ("X-IBM-Data-Type": encoding ). And will pass Content-Type: application/octet-stream for binary data.
        • DOWNLOAD: applies Content-Type header.
        • Default (no context): applies the default content-type header: {"X-IBM-Data-Type": "text"}
    ZosFilesHeaders.generateHeaders<T>({
      options,
      context,
      dataLength,
    }: {
      options: T;
      context?: ZosFilesContext;
      dataLength?: number | string;
    }): IHeaderContent[]

Because of this centralization, organization, and newly provided clear definitions for expected headers, any future header changes or additions require modifications in only one location. Creating headers can now be done with a single function call, without much thought required in future development of the methods within ZosFiles SDK.

@ATorrise ATorrise requested review from zFernand0, jace-roell and awharn and removed request for zFernand0 and jace-roell February 14, 2025 21:58
Copy link

github-actions bot commented Feb 14, 2025

📅 Suggested merge-by date: 3/5/2025

Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 97.39583% with 5 lines in your changes missing coverage. Please review.

Project coverage is 91.45%. Comparing base (26d6572) to head (8fa47b2).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
packages/zosfiles/src/utils/ZosFilesHeaders.ts 95.96% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2436      +/-   ##
==========================================
- Coverage   91.50%   91.45%   -0.06%     
==========================================
  Files         641      642       +1     
  Lines       18419    18423       +4     
  Branches     3963     3872      -91     
==========================================
- Hits        16855    16848       -7     
- Misses       1562     1573      +11     
  Partials        2        2              

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Amber Torrise <[email protected]>
@zFernand0
Copy link
Member

zFernand0 commented Mar 6, 2025

I'm trying to keep track of the test in the hopes that it would give us some perspective.

Nested if approach:
80bef2d
image

Cleaner switch approach:
c9e76a5
image

Aftr merging master (adf4304)
image


Personally, switch > if for complex stuff like this 😋

@ATorrise ATorrise requested a review from zFernand0 March 10, 2025 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review/QA
Development

Successfully merging this pull request may close these issues.

Add support for responseTimeout to other MVS and USS SDK functions
6 participants