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

Container Identifiers #58

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Conversation

dimitribouniol
Copy link
Contributor

@dimitribouniol dimitribouniol commented May 2, 2024

These changes are now available in 4.2.0

To help users who need to configure a production and development container, added two new containerIDs: production and development. Also added documentation and clarified how the default ID is used, which isn't always the default depending on how use is called.

Note: Since this is new, I went with development instead of sandbox here because all of Apple's documentation refers to it as the development environment. We should probably add .development and deprecate .sandbox in APNSwift as well.

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

One minor addition

/// You must configure at lease one client in order to send notifications to devices. If you plan on supporting both development builds (ie. run from Xcode) and released builds (ie. TestFlight/App Store), you must configure at least two configurations:
///
/// ```swift
/// let apnsKey = ... // The .p8 file as a string.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a note here to suggest passing it in as an environment variable and not hardcoding it in the source code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout, I’ll add a section for security considerations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@0xTim 0xTim added the semver-minor Contains new APIs label May 4, 2024
@dimitribouniol dimitribouniol force-pushed the dimitri/identifiers branch from b9416fb to e7db3f9 Compare May 4, 2024 17:08
@dimitribouniol dimitribouniol force-pushed the dimitri/identifiers branch from e7db3f9 to 3b4f6e3 Compare May 7, 2024 11:57
@dimitribouniol dimitribouniol requested a review from 0xTim August 18, 2024 06:14
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 0% with 25 lines in your changes missing coverage. Please review.

Project coverage is 64.44%. Comparing base (98ba2f2) to head (965e0b2).
Report is 2 commits behind head on main.

Files Patch % Lines
Sources/VaporAPNS/Application+APNS.swift 0.00% 21 Missing ⚠️
Sources/VaporAPNS/APNSContainerID.swift 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #58       +/-   ##
===========================================
- Coverage   89.39%   64.44%   -24.95%     
===========================================
  Files           4        4               
  Lines          66       90       +24     
===========================================
- Hits           59       58        -1     
- Misses          7       32       +25     
Files Coverage Δ
Sources/VaporAPNS/APNSContainers.swift 86.84% <ø> (-0.34%) ⬇️
Sources/VaporAPNS/APNSContainerID.swift 33.33% <0.00%> (-66.67%) ⬇️
Sources/VaporAPNS/Application+APNS.swift 47.72% <0.00%> (-43.58%) ⬇️

@0xTim 0xTim merged commit dedccc1 into vapor:main Aug 20, 2024
8 checks passed
@dimitribouniol dimitribouniol deleted the dimitri/identifiers branch August 20, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Contains new APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants