Skip to content

Add AWS IAM authorization support #64

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

Merged
merged 1 commit into from Aug 10, 2021
Merged

Add AWS IAM authorization support #64

merged 1 commit into from Aug 10, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jul 20, 2021

No description provided.

Copy link
Owner

@mghoneimy mghoneimy left a comment

Choose a reason for hiding this comment

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

I have left a few comments specific to design and implementation. A more general comment is that none of the tests are updated. We need to update the test classes to verify the modified functionalities.

@ghost
Copy link
Author

ghost commented Aug 2, 2021

@mghoneimy thank you for a review. Fixed the comments, but some things are to be discussed, looking forward.

Copy link
Owner

@mghoneimy mghoneimy left a comment

Choose a reason for hiding this comment

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

We are very close here. The comments I added are mostly small. I'm trying to figure out why travis builds stopped on build requests too. Once both these things are done, we should be good to go.

@ghost
Copy link
Author

ghost commented Aug 3, 2021

Tests are fixed now

Copy link
Owner

@mghoneimy mghoneimy left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all the previous comments and fixing the tests!

I think that one comment is was not completely addressed and I think that I found a problem worth looking into before we merge this in.

@mghoneimy
Copy link
Owner

Can you rebase your branch on the latest updates on master please, I had to replace travis-ci with Github actions to have the tests continue to run.

@mghoneimy
Copy link
Owner

Thanks for addressing all the comments. I accepted the changes, but again, I still need need you to rebase your changes on master before we can merge the PR.

@ghost
Copy link
Author

ghost commented Aug 10, 2021

Thanks for addressing all the comments. I accepted the changes, but again, I still need need you to rebase your changes on master before we can merge the PR.

Great! Thank you.
Yeah, no problem. I've rebased the branch.
Checks were not started, please, run them to ensure everything is fine

@mghoneimy mghoneimy merged commit caadeba into mghoneimy:master Aug 10, 2021
@ghost ghost deleted the add_iam_auth_support branch September 28, 2021 15:06
@longmtran
Copy link

Hi, does anyone have an example of how the code should look like with this? As in, when making a query to a GraphQL endpoint with IAM auth.

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