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

[UPD] Remove exposed Key, Add version Param, Deprecated support for python 2 #20

Merged
merged 9 commits into from
Oct 25, 2021

Conversation

mashanz
Copy link
Contributor

@mashanz mashanz commented Oct 22, 2021

This PR will fix view issue here:

  1. There a lot exposed Key that may be abused
  2. Add version param so the user can call midtransclient.__version__ to check this modul version for better version control & integration or backward compatibility.
  3. Remove python 2 support and add python 3.10

@rizdaprasetya
Copy link
Collaborator

Thanks for the quick PR! Mostly looks good, we'll be reviewing this in-depth sometimes on next weekday. Some quick comments:

There a lot exposed Key that may be abused

It is actually intentional for the keys to be hardcoded, so that:

  • example flask app can be easily run without requiring people to sign up first, it is good for demonstration purpose.
  • test result can be consistent and centralized to one test account
    So it is fine to leave the keys exposed. We acknowledge & are ok with the risk. It is also just a sandbox env key with no major potential harm (which anyway we can block anytime from our side if we need to).

We'll be back soon to further review & merge, thanks!

@mashanz
Copy link
Contributor Author

mashanz commented Oct 22, 2021

I see, I thought it is a mistakes, There are no explanation why the key are there to begin with.
Thanks for the comments and explanation.

This reverts commit 6215c4e.

It is actually intentional for the keys to be hardcoded, so that:
- example flask app can be easily run without requiring people to sign up first, it is good for demonstration purpose.
- test result can be consistent and centralized to one test account
So it is fine to leave the keys exposed. We acknowledge & are ok with the risk. It is also just a sandbox env key with no major potential harm (which anyway we can block anytime from our side if we need to).
Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

Overall the PR looks good 👍 thanks! I reverted the API keys removal, fixed test, and added some notes to improve this PR. Should be good now.

We'll merge this now (but not release it yet, we'll release together with that 1 other PR about Python 2 compatibility). Thanks for your contribution @mashanz ! Appreciate your solid works! 😃

@rizdaprasetya rizdaprasetya merged commit 3c482b8 into Midtrans:master Oct 25, 2021
@rizdaprasetya rizdaprasetya added the hacktoberfest-accepted Accepted PR for hacktoberfest participant label Oct 25, 2021
@mashanz
Copy link
Contributor Author

mashanz commented Oct 25, 2021

Thanks @rizdaprasetya 🔥🎉👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted PR for hacktoberfest participant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants