Add required property to defineConditionKeys to be used for serviceResolvedConditionKeys #2183

0xjjoyy · 2024-03-13T06:17:08Z

Background

What do these changes do?

Adds a required property to the IAM trait defineConditionKeys.
Updates the IAM trait documentation.

Why are they important?

Defines whether a service resolved condition key is required. Not applicable to request resolved condition keys as the native @required trait must be used on the field directly.

Testing

How did you test these changes?

Smithy test files.
Java test class to check required is set correctly.

Links

Links to additional context, if necessary
Issue #, if applicable (see here for a list of keywords to use for linking issues)

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

…solvedConditionKeys

kstich · 2024-03-14T16:31:10Z

docs/source-2.0/aws/aws-iam.rst

+    * - required
+      - ``boolean``
+      - Defines whether a service resolved condition key is required. Not 
+        applicable to request resolved condition keys as the native 


The "native @required" should be replaced with a :ref: to the required trait.

Suggested change

applicable to request resolved condition keys as the native

applicable to request resolved condition keys, as the native

There should be some sort of validation for if this property is set and the named key is request-resolved. I think that case should fail and direct users to applying the @required trait instead.

Still missing the :ref: for the required trait and the validation wasn't added.

Still missing the :ref: for the required trait and the validation wasn't added.

Added

There should be some sort of validation for if this property is set and the named key is request-resolved. I think that case should fail and direct users to applying the @required trait instead.

Added the validator

kstich · 2024-03-14T16:32:24Z

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeyDefinition.java

@@ -91,13 +103,16 @@ public Optional<String> getRelativeDocumentation() {
        return Optional.ofNullable(relativeDocumentation);
    }

+    public Optional<Boolean> getRequired() { return Optional.ofNullable(required); }


Since required is a primitive boolean, it's not nullable. This should just be a boolean isRequired() method that returns the value.

Updated to be isRequired without nullable

kstich · 2024-03-14T16:32:45Z

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeyDefinition.java


    private final String type;
    private final String documentation;
    private final String externalDocumentation;
    private final String relativeDocumentation;

+    /**


These should be part of the javadoc for the isRequired method and not on the private var.

Updated javadoc to be on the method

kstich · 2024-03-14T16:33:21Z

smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy

@@ -119,6 +119,10 @@ structure ConditionKeyDefinition {
    /// A relative URL path that defines more information about the condition key
    /// within a set of IAM-related documentation.
    relativeDocumentation: String
+
+    // Whether a service resolved condition key is required
+    // Not applicable to request resolved condition key as the native @required trait must be used


Suggested change

// Not applicable to request resolved condition key as the native @required trait must be used

// Not applicable to request resolved condition keys, as the native @required trait must be used.

Wasn't updated.

smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy

kstich · 2024-03-14T16:34:15Z

...traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy

+)
+service MyService {
+    version: "2017-02-11"
+}


Should have empty line at EOF.

kstich · 2024-03-14T16:35:31Z

...re/amazon/smithy/aws/iam/traits/errorfiles/define-conditionkeys-required-booleancheck.smithy

@@ -0,0 +1,19 @@
+$version: "2"


I don't think we need this test, as it's not validating behavior specific to the trait - it's validation for model loading and parsing.

kstich · 2024-03-14T16:43:33Z

It appears there are also checkstyle validations that are failing the build.

typo Co-authored-by: Kevin Stich <[email protected]>

…am.smithy period Co-authored-by: Kevin Stich <[email protected]>

...s-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeyDefinition.java

kstich · 2024-04-09T05:13:46Z

smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy

@@ -119,6 +119,10 @@ structure ConditionKeyDefinition {
    /// A relative URL path that defines more information about the condition key
    /// within a set of IAM-related documentation.
    relativeDocumentation: String
+
+    // Whether a service resolved condition key is required
+    // Not applicable to request resolved condition key as the native @required trait must be used


Wasn't updated.

…/iam/traits/ConditionKeyDefinition.java Co-authored-by: Kevin Stich <[email protected]>

…keys use the required property

kstich · 2024-05-15T07:03:51Z

Resolving in favor of #2288

0xjjoyy added 2 commits March 13, 2024 01:54

Add required property to defineConditionKeys to be used for serviceRe…

e969e64

…solvedConditionKeys

adding required to IAM trait docs for defineConditionKeys

49e101d

0xjjoyy requested a review from a team as a code owner March 13, 2024 06:17

0xjjoyy requested a review from AndrewFossAWS March 13, 2024 06:17

kstich requested changes Mar 14, 2024

View reviewed changes

0xjjoyy and others added 6 commits April 8, 2024 12:49

Update docs/source-2.0/aws/aws-iam.rst

3a1aa91

typo Co-authored-by: Kevin Stich <[email protected]>

primitive boolean isRequired

fc0b889

moving javadoc to method

7a88ca1

Update smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.i…

438615e

…am.smithy period Co-authored-by: Kevin Stich <[email protected]>

add EOF

94b4d8e

fix checkstyle errors

e1d13df

kstich reviewed Apr 9, 2024

View reviewed changes

0xjjoyy and others added 8 commits April 9, 2024 08:29

typo

949e839

Update smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws…

ac55de3

…/iam/traits/ConditionKeyDefinition.java Co-authored-by: Kevin Stich <[email protected]>

Merge branch 'smithy-lang:main' into required-property

f6deba1

remove define-conditionkeys-required-booleancheck.smithy

2bfcb2a

typo

a01f67a

add ref for requried trait

021c184

add DefineConditionKeysTraitValidator to check only service resolved …

9514eb4

…keys use the required property

remove newline

3160a7e

0xjjoyy requested a review from kstich April 9, 2024 17:06

update license header

b736b45

kstich mentioned this pull request May 15, 2024

Add required for service resolved condition keys #2288

Merged

kstich closed this May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add required property to defineConditionKeys to be used for serviceResolvedConditionKeys #2183

Add required property to defineConditionKeys to be used for serviceResolvedConditionKeys #2183

0xjjoyy commented Mar 13, 2024

kstich Mar 14, 2024

kstich Mar 14, 2024

kstich Mar 14, 2024

kstich Apr 9, 2024 •

edited

Loading

0xjjoyy Apr 9, 2024

0xjjoyy Apr 9, 2024

kstich Mar 14, 2024

0xjjoyy Apr 8, 2024

kstich Mar 14, 2024

0xjjoyy Apr 8, 2024

kstich Mar 14, 2024

0xjjoyy Apr 8, 2024

kstich Apr 9, 2024

0xjjoyy Apr 9, 2024

kstich Mar 14, 2024

0xjjoyy Apr 8, 2024

kstich Mar 14, 2024

0xjjoyy Apr 9, 2024

kstich commented Mar 14, 2024

kstich Apr 9, 2024

kstich commented May 15, 2024

	applicable to request resolved condition keys as the native
	applicable to request resolved condition keys, as the native

	// Not applicable to request resolved condition key as the native @required trait must be used
	// Not applicable to request resolved condition keys, as the native @required trait must be used.

Add required property to defineConditionKeys to be used for serviceResolvedConditionKeys #2183

Add required property to defineConditionKeys to be used for serviceResolvedConditionKeys #2183

Conversation

0xjjoyy commented Mar 13, 2024

Background

Testing

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstich Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstich commented Mar 14, 2024

Choose a reason for hiding this comment

kstich commented May 15, 2024

kstich Apr 9, 2024 •

edited

Loading