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: add admin roles which have admin control over a graphql api #8601

Merged
merged 1 commit into from
Oct 30, 2021

Conversation

SwaySway
Copy link
Contributor

@SwaySway SwaySway commented Oct 30, 2021

Description of changes

  • extend admin ui role check to work with functions which have access to graphql apis
  • updated unit/e2e
  • change the options for the auth transformer class to only require an admin role list to determine if the iam directives should added on a @model

Description of how you validated changes

  • unit/e2e

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@SwaySway SwaySway requested a review from a team as a code owner October 30, 2021 02:55
lazpavel
lazpavel approved these changes Oct 30, 2021
@lazpavel lazpavel self-requested a review October 30, 2021 03:18
@lgtm-com
Copy link

lgtm-com bot commented Oct 30, 2021

This pull request introduces 13 alerts when merging 335628d into 1d8ff73 - view on LGTM.com

new alerts:

  • 13 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Oct 30, 2021

This pull request introduces 13 alerts when merging a420641 into 1d8ff73 - view on LGTM.com

new alerts:

  • 13 for Unused variable, import, function or class

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2021

Codecov Report

Merging #8601 (28b9a32) into master (bd435e2) will increase coverage by 1.20%.
The diff coverage is 56.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8601      +/-   ##
==========================================
+ Coverage   55.92%   57.12%   +1.20%     
==========================================
  Files         695      728      +33     
  Lines       38881    41174    +2293     
  Branches     7871     8441     +570     
==========================================
+ Hits        21743    23520    +1777     
- Misses      16343    16842     +499     
- Partials      795      812      +17     
Impacted Files Coverage Δ
...fy-appsync-simulator/src/schema/directives/auth.ts 18.42% <ø> (ø)
...ppsync-simulator/src/utils/auth-helpers/helpers.ts 80.95% <ø> (ø)
...egory-api/src/provider-utils/supported-services.ts 100.00% <ø> (ø)
packages/amplify-category-auth/src/index.js 15.74% <0.00%> (ø)
...ider-utils/awscloudformation/assets/string-maps.js 86.20% <ø> (ø)
...auth/src/provider-utils/awscloudformation/index.js 3.90% <0.00%> (ø)
...ils/awscloudformation/service-walkthrough-types.ts 100.00% <ø> (ø)
...udformation/service-walkthroughs/auth-questions.js 3.33% <0.00%> (ø)
...s/amplify-category-geo/src/commands/geo/console.ts 53.84% <0.00%> (+3.84%) ⬆️
...amplify-cli-core/src/feature-flags/featureFlags.ts 83.62% <ø> (ø)
... and 110 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a0fda8...28b9a32. Read the comment docs.

Copy link
Contributor

@lazpavel lazpavel left a comment

Choose a reason for hiding this comment

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

LGTM, can you fix the bot alerts

@SwaySway
Copy link
Contributor Author

bot alerts addressed

@lgtm-com
Copy link

lgtm-com bot commented Oct 30, 2021

This pull request introduces 1 alert when merging 3846759 into 4a0fda8 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@SwaySway SwaySway merged commit 4d50df0 into aws-amplify:master Oct 30, 2021
ammarkarachi pushed a commit to ammarkarachi/amplify-cli that referenced this pull request Nov 3, 2021
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.

4 participants