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

Adding IAM Action name trait to override using the API operation name #1665

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

0xjjoyy
Copy link
Contributor

@0xjjoyy 0xjjoyy commented Mar 7, 2023

Issue #, if available:

Description of changes:
Adds the ActionName trait to override using the API operation name as the IAM action name. Also, allows adding additional actions using the input member field if the value is not null in the request.

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

@0xjjoyy 0xjjoyy requested a review from a team as a code owner March 7, 2023 20:11
Copy link
Contributor

@kstich kstich left a comment

Choose a reason for hiding this comment

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

@@ -0,0 +1,3 @@
gradle/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove this, the top level .gitignore should handle these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -5,3 +5,4 @@ software.amazon.smithy.aws.iam.traits.DisableConditionKeyInferenceTrait$Provider
software.amazon.smithy.aws.iam.traits.RequiredActionsTrait$Provider
software.amazon.smithy.aws.iam.traits.SupportedPrincipalTypesTrait$Provider
software.amazon.smithy.aws.iam.traits.IamResourceTrait$Provider
software.amazon.smithy.aws.iam.traits.ActionNameTrait$Provider
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should have a trailing newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

},
"traits": {
"smithy.api#trait": {
"selector": ":test(operation, member)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this should be "operation", given the documentation. Tests should be updated to reflect this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,17 @@
$version: "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a v2 model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@kstich kstich self-assigned this Mar 7, 2023
@0xjjoyy
Copy link
Contributor Author

0xjjoyy commented Mar 8, 2023

Thanks, made updates.

This will need updates to the specification, that'd be in aws-iam.rst in the docs folder.

updated

Java files should use the short-form license header (see #1614)

updated

@0xjjoyy 0xjjoyy requested a review from kstich March 8, 2023 04:56
.. smithy-trait:: aws.iam#actionName
.. _aws.iam#actionName-trait:

-------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

The - wrapping needs to be the same length as the header content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

-------------------------------

Summary
Provides the ability to override the default action name for the operation. By convention, the action name is the same as the operation name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you wrap the new contents at 80 chars like the rest of the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify the summary to "Provides a custom IAM action name." since the rest is duplicate information with somewhere else in the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Summary
Provides the ability to override the default action name for the operation. By convention, the action name is the same as the operation name.
Trait selector
``:test(operation)``
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't need to be wrapped in :test().

"smithy.api#trait": {
"selector": "operation"
},
"smithy.api#documentation": "Provides the ability to override the default action name for the operation. By convention, the action name is the same as the operation name."
Copy link
Contributor

Choose a reason for hiding this comment

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

s/convention/default

Replace the first sentence with the one recommended for the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Value type
``string``

Operations not annotated with the ``actionName`` trait use the operation name as the action name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults to the :ref:`shape name of the shape ID ` of the targeted operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


The following example defines two operations:

* OperationA is not annoated with the ``actionName`` trait, so has the action name ``OperationA`` which is the same as the operation name.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The first OperationA should be in double backticks.
  • s/annoated/annotated
  • "so has the action name OperationA which is the same as the operation name." -> and resolves the action name of OperationA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

The following example defines two operations:

* OperationA is not annoated with the ``actionName`` trait, so has the action name ``OperationA`` which is the same as the operation name.
* OperationB has the ``actionName`` trait, so has the action name ``OverridingActionName``.
Copy link
Contributor

Choose a reason for hiding this comment

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

OperationB should be in double backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@kstich kstich merged commit 91d0af9 into smithy-lang:main Mar 29, 2023
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