c12i / mpesa-rust

A MPESA API sdk in Rust
https://c12i.github.io/mpesa-rust/mpesa/index.html
MIT License
43 stars 14 forks source link

Minor improvements and less lifetime & generic type noise #95

Closed Tevinthuku closed 8 months ago

Tevinthuku commented 8 months ago

Prior to this change, all the Request builders had to have the ApiEnvironment generic parameter which felt a bit off. (RequestBuilders shouldn't concern themselves with the APIEnvironment)

I've refactored the MpesaClient abit, instead of holding the Trait, to just holds the values it gets from theApiEnvironment trait implementors.

The result should ideally be, less noise, which makes the code a bit easier to read.

Closes #94

Tevinthuku commented 8 months ago

Looking at one of the passing pipelines https://github.com/c12i/mpesa-rust/actions/runs/7127587401/job/19407792529?pr=93 I think the issue might be with the Client keys

image


On my PR they are empty https://github.com/c12i/mpesa-rust/actions/runs/7372505901/job/20060896287?pr=95
image

Idk if this means that I should have valid keys in my fork 🤷🏽

c12i commented 8 months ago

Idk if this means that I should have valid keys in my fork 🤷🏽

Yes, they are needed, unfortunately. This was as a result of a recent change we introduced that requires them, will document this better for collaborators.

c12i commented 8 months ago

Here is the link to where to create your app and get your key: https://developer.safaricom.co.ke/MyApps

Note that even if you do add the required keys, there is still a small likelihood that the doc tests might still fail.

The doc tests are not black boxed, they actually make calls to the real API. This was intentionally set up to allow us to catch any breaking change from safaricom's end, since they don't do a great job in communicating these changes. All other tests mock the actual api but they end up becoming stale when unexpected changes are made to the live API. We are looking to improve this workflow though.

Tevinthuku commented 8 months ago

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

Might be of interest to you. Setting the keys on my fork didnt work

On Mon, 1 Jan 2024 at 13:02, Collins Muriuki @.***> wrote:

Here is the link to where to create your app and get your key: https://developer.safaricom.co.ke/MyApps

Note that even if you do add the required keys, there is still a small likelihood that the doc tests might still fail.

The doc tests are not black boxed, they actually make calls to the real API. This was intentionally set up to allow us to catch any breaking change from safaricom's end, since they don't do a great job in communicating these changes. All other tests mock the actual api but they end up becoming stale when unexpected changes are made to the live API. We are looking to improve this workflow though.

— Reply to this email directly, view it on GitHub https://github.com/c12i/mpesa-rust/pull/95#issuecomment-1873255321, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC4Q7GOUJHCVJUR3HOC3K5LYMKCR3AVCNFSM6AAAAABBIOU44WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZTGI2TKMZSGE . You are receiving this because you authored the thread.Message ID: @.***>

c12i commented 8 months ago

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks Might be of interest to you.

Thanks for sharing, I have updated the rules. Pipeline will need to be re-triggered so that I can approve it.

c12i commented 8 months ago

will merge this PR for now, noticed my pipelines on master were failing similarly after merging #93, reshuffled the keys and resolved the issue (the flaky tests still cause failures), but this is tolerable