Skip to content

Header generation refactoring #2436

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

Closed
wants to merge 63 commits into from
Closed

Header generation refactoring #2436

wants to merge 63 commits into from

Conversation

ATorrise
Copy link
Contributor

@ATorrise ATorrise commented Feb 14, 2025

This refactoring effort centralizes and standardizes header generation across the Files 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:

    • Requires:
      • the request options
    • Optionally takes:
      • dataLength parameter to add a Content-Length header when needed.
      • ZosFilesContext enum that guides which Content-Type headers are applied for certain methods. These 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.
        • No context (default)
          • 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 header expectations, 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 (a1b71ff) to head (9b6d751).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
packages/zosfiles/src/utils/ZosFilesHeaders.ts 96.03% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2436      +/-   ##
==========================================
- Coverage   91.51%   91.45%   -0.06%     
==========================================
  Files         641      642       +1     
  Lines       18425    18430       +5     
  Branches     4002     3990      -12     
==========================================
- Hits        16861    16855       -6     
- 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
@ATorrise ATorrise marked this pull request as draft March 12, 2025 15:39
@zowe-robot zowe-robot moved this from Review/QA to In Progress in Zowe CLI Squad Mar 12, 2025
@ATorrise
Copy link
Contributor Author

momentarily moving this to draft status for a few hours to make a significant change based on sonar cloud's good suggestion

@ATorrise ATorrise marked this pull request as ready for review March 14, 2025 19:06
@zowe-robot zowe-robot moved this from In Progress to Review/QA in Zowe CLI Squad Mar 14, 2025
@ATorrise ATorrise linked an issue Mar 18, 2025 that may be closed by this pull request
ATorrise and others added 2 commits March 19, 2025 10:51
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
Copy link

private static headerMap = new Map<string, <T>(options: T, context?: ZosFilesContext) => IHeaderContent | IHeaderContent[]>();
static initializeHeaderMap() {
this.headerMap.set("binary", (options) => (options as any).binary === true ? ZosmfHeaders.X_IBM_BINARY : undefined);
this.headerMap.set("record", (options) => (options as any).binary !== true ? ZosmfHeaders.X_IBM_RECORD : undefined);
Copy link
Member

Choose a reason for hiding this comment

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

The system tests are failing because many data sets are being uploaded with a record header. This line seems to be setting the record header if the data set is not being transferred in binary mode. I believe that instead it should only be setting the record header if the record option is true.

@ATorrise
Copy link
Contributor Author

closing this PR and opening a new PR without the enhancement that only provides a fix for the RTO header

@ATorrise ATorrise closed this Mar 21, 2025
@github-project-automation github-project-automation bot moved this from Review/QA to Closed in Zowe CLI Squad Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

6 participants