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

Remove legacy node-fetch and document mod/cloudfront ☁️ #1645

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

RobAndrewHurst
Copy link
Contributor

Description

I have removed the node-fetch reference in the cloudfront.js mod.

I also added in documentation for the script.

GitHub Issue

#1641

Type of Change

Please delete options that are not relevant, and select all options that apply.

  • ✅ Bug fix (non-breaking change which fixes an issue)
  • ✅ Documentation

Code Quality Checklist

  • ✅ My code follows the guidelines of XYZ
  • ✅ My code has been commented
  • ✅ Documentation has been updated
  • ✅ New and existing unit tests pass locally with my changes
  • ✅ Main has been merged into this PR

@RobAndrewHurst RobAndrewHurst linked an issue Oct 31, 2024 that may be closed by this pull request
@RobAndrewHurst RobAndrewHurst added Bug A genuine bug. There must be some form of error exception to work with. Code Issues related to the code structure and performance. labels Oct 31, 2024
@dbauszus-glx dbauszus-glx changed the title Remove legacy Fetch and Document Cloudinary ☁️ mod Remove legacy node-fetch and document mod/cloudfront ☁️ Oct 31, 2024
@dbauszus-glx
Copy link
Member

I have updated the docs. There was some heavy nesting on the properties which were defined as params.

image

This is now cleaned up.

image

I also added the module requires to the module block.

image

@dbauszus-glx
Copy link
Member

I still want to test this failing. I think the try...fetch is just there to throw an error if the key is undefined or fails to sign. I want to actually test this providing a false key and a false resource.

@GEOLYTIX GEOLYTIX deleted a comment Oct 31, 2024
Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

#1542 must be merged into this PR.

The provider module must return an Error rather than text failed to retrieve resource.

The cloudfront module must export null if the cloudfront key is not defined rather than throwing an Error which isn't caught.

The cloudfront module must return an Error and not just throw and Error with no return if the resource is not available or failed to resource.

The urlSigner should be a signer module to be imported by the provider module for internal use. signedUrl request should be rerouted to the cloudfront signer module.

@dbauszus-glx dbauszus-glx self-requested a review November 1, 2024 10:10
Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

This is good to go. I create a seperate issue for the code structure and security concerns.

#1651

@RobAndrewHurst RobAndrewHurst linked an issue Nov 1, 2024 that may be closed by this pull request
Copy link

sonarqubecloud bot commented Nov 1, 2024

@RobAndrewHurst RobAndrewHurst changed the base branch from patch to minor November 1, 2024 10:57
@RobAndrewHurst RobAndrewHurst changed the base branch from minor to patch November 1, 2024 10:58
@RobAndrewHurst RobAndrewHurst merged commit e61199b into GEOLYTIX:patch Nov 1, 2024
6 checks passed
@RobAndrewHurst RobAndrewHurst deleted the node-fetch branch November 1, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with. Code Issues related to the code structure and performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloudfront Signer Cloudfront Provider; use of node-fetch
2 participants