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

Feat/report token and failures #73

Closed
wants to merge 8 commits into from
Closed

Feat/report token and failures #73

wants to merge 8 commits into from

Conversation

powerje
Copy link
Contributor

@powerje powerje commented Jun 10, 2020

Adds optional delegate methods for getting the push token and any errors that occur while registering for push tokens, per discussion in #43

@powerje
Copy link
Contributor Author

powerje commented Jun 10, 2020

I'm not sure what the build failure is here - any way I can re-create the build process locally to see?

@powerje
Copy link
Contributor Author

powerje commented Jun 10, 2020

I'm having issues pulling this in via Xcode. It looks like SPM isn't getting values from Global.xcconfig so SDK_VERSION isn't set and this fails in MSInstallationManager.m:

NSString *sdkVersion = [NSString stringWithUTF8String:SDK_VERSION];

I don't see a way of incorporating the xcconfig in the SPM manifest. I think we may need to rethink how SDK_VERSION is defined - perhaps it could be a macro in a header file instead?

@mpodwysocki
Copy link
Member

@powerje I'm open to any changes we need to do. Let's look to AppCenter for how they did it.

Added TODO to work around SDK_VERSION xcconfig issue with SPM
@mpodwysocki
Copy link
Member

@powerje

I'm not sure what the build failure is here - any way I can re-create the build process locally to see?

It's an issue with the BVTs (Build Verification Tests), and I fixed it as per #72 so you should be able to have the builds work now if you get latest

* @param notificationHub The instance of MSNotificationHub
* @param pushToken The push token
*/
- (void)notificationHub:(MSNotificationHub *)notificationHub
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to do a rename, we should have maybe a sample linked in our Swift sample app?

@powerje
Copy link
Contributor Author

powerje commented Jul 6, 2020

@mpodwysocki apologies on not getting to the requested changes yet. Let me know if there's any urgency on Microsoft's end to get these in sooner than later. I will be able to allocate time for this around the 22nd based on my sprint schedule but could move it up in priority if need be to hit a Microsoft release target.

@mpodwysocki
Copy link
Member

@powerje We're looking to GA fairly soon, although this is not a breaking change at this point, but an addition, so it can certainly be available after that. We're also adding push to user and other features so there are opportunities forthcoming.

@mpodwysocki
Copy link
Member

@mpodwysocki apologies on not getting to the requested changes yet. Let me know if there's any urgency on Microsoft's end to get these in sooner than later. I will be able to allocate time for this around the 22nd based on my sprint schedule but could move it up in priority if need be to hit a Microsoft release target.

As you can tell, we've made a lot of changes since your initial PR. Can you update it to reflect the newest changes?

@powerje powerje closed this Aug 10, 2020
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