feat: create Java cfn-response equivalent (#558) #560

bdkosher · 2021-09-28T13:16:22Z

Introduce Java implementation of the cfn-response module based on NodeJS design.

Issue #, if available:

Addresses #558

Description of changes:

Add new powertools-cloudformation Maven module to house the sole Java class, CloudFormationResponse.
Include one new parent POM dependency on software.amazon.awssdk:http-client-spi:${aws.sdk.version}.
Add Unit Test class with 100% code coverage.

Checklist

Meet tenets criteria
Update tests
Update docs
PR title follows conventional commit semantics

Breaking change checklist

None. New code.

RFC issue #:

Migration process documented
Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

bdkosher · 2021-09-28T14:37:15Z

Is the SpotBugs / codecheck failure a false positive?

The logs do not show any violations for the impacted module:
/home/runner/work/aws-lambda-powertools-java/aws-lambda-powertools-java/powertools-cloudformation/target/spotbugsXml.xml has 0 violations

msailes · 2021-09-30T09:03:27Z

A couple of questions from me.

Do you think it would be useful to include a builder for ResponseBody this would give customers a similar feel to using the v2 SDK and other AWS Java libraries?
Can you add documentation for this module so it appears on the website - https://awslabs.github.io/aws-lambda-powertools-java/

bdkosher · 2021-09-30T12:39:43Z

ResponseBody is currently more of an internal object for producing the desired JSON payload. We could directly expose that object for maximal control and have that be passed to a singular "send" method instead of having various polymorphic "send"s with different parameters. Alternatively, we could make the overall class follow the builder pattern, culminating in a "build" that would actually perform the "send". I do like the idea of having a builder here one way or another. I'm not particularly fond that I cannot specify a noSend arg without having to explicitly specify a physicalResourceId arg as well.
Certainly! I totally didn't notice the docs folder earlier.

Another design issue for your consideration: Right now there's a single ObjectMapper that arguably does double-duty: format the overall JSON in the HTTP request and format the Object responseData that gets included in that payload.

For my own application, e.g. I would ideally like to pass a software.amazon.awssdk.services.chime.model.CreateAppInstanceResponse as the "responseData", but this class does not serialize nicely. I could pass in my own ObjectMapper that would support serializing this type, but then that might interfere with overall JSON serialization (which is why I've currently made the constructor that accepts the ObjectMapper arg protected). Some ideas:

Instead of a generic Object arg, use Map<String, Object> or possibly Map<String, String> to better mimic the "list of name value pairs" described in the JS/Python versions.
Allow a custom ObjectMapper to be provided on init, but keep it separate from the generic ObjectMapper used to produce the overall JSON (might be a bit tricky to implement)
Or just leave things the way they are.

pankajagrawal16 · 2021-10-01T07:28:14Z

Is the SpotBugs / codecheck failure a false positive?

The logs do not show any violations for the impacted module: /home/runner/work/aws-lambda-powertools-java/aws-lambda-powertools-java/powertools-cloudformation/target/spotbugsXml.xml has 0 violations

Yeah I have been trying to figure this one out. It looks like it fails on PRs where author donot have access to repo. I need to find some time to troubleshoot this further.

powertools-cloudformation/pom.xml

...n/src/main/java/software/amazon/lambda/powertools/cloudformation/CloudFormationResponse.java

pankajagrawal16 · 2021-10-01T07:36:11Z

...n/src/main/java/software/amazon/lambda/powertools/cloudformation/CloudFormationResponse.java

+     *
+     * @param client HTTP client to use for sending requests; cannot be null
+     */
+    public CloudFormationResponse(SdkHttpClient client) {


Will be good to provide an implementation where user dont need to care about passing http client as well and module can take care of setting it up.

From Custom resource perspective, I believe it will provide better developer experience.

True. The JS and Python versions embed the client. We'd just need to

Pick a particular client impl (Apache?) and

Include it as a dependency to the project.

pankajagrawal16 · 2021-10-01T08:11:45Z

@bdkosher Firstly, Thanks for opening the issue and PR.

I am thinking out loud here but will it be better builder experience if we provide abstract implementation like this:

public abstract class CloudformationCustomResource {
    
    public Object handleRequest(CloudFormationCustomResourceEvent input, Context context) {
        Response response = null;

        switch (input.getRequestType()) {
            case "CREATE":
                response = create(input, context);
                buildAndSendCfnResponse(response, input, context);
                break;
            case "UPDATE":
                response = update(input, context);
                buildAndSendCfnResponse(response, input, context);
                break;
            // and so on
        }

        return response;
    }

    protected void buildAndSendCfnResponse(Response response, CloudFormationCustomResourceEvent input, Context context) {
        // Map Response to Response Body
        // Send response
    }

    public abstract Response create(CloudFormationCustomResourceEvent input, Context context);

    public abstract Response update(CloudFormationCustomResourceEvent input, Context context);

    public abstract Response delete(CloudFormationCustomResourceEvent input, Context context);
}

Then from builder perspective, we can control alot of things, for example, Response object can have all the builder goodness and be very specific to what we expect users to provide us. It can be a good place to capture mapper as which they would want us to use to serialize response data.

Also then builders wont need to know too much details about how to handle custom resource events etc. From their perspective, they just need to do their stuff for create, update and delete and provide us a response object.

What do you think? I believe this is closer to the implementation we have for python one.

Builder experience then looks something like this:

public class CustomResourceHandler extends CloudformationCustomResource {


    @Override
    public Response create(CloudFormationCustomResourceEvent input, Context context) {
        return null;
    }

    @Override
    public Response update(CloudFormationCustomResourceEvent input, Context context) {
        return null;
    }

    @Override
    public Response delete(CloudFormationCustomResourceEvent input, Context context) {
        return null;
    }
}

bdkosher · 2021-10-01T15:53:10Z

Seems like a good idea; as far as I know, CloudFormationResponse only gets used in the context of a handler that does some sort of one-time stack setup action based on the event type. So if I understand you correctly, we'd add two more classes:

An abstract base handler class that would allow subclasses to just "do their stuff for create, update and delete and provide us a response object"
1. Should we have the create/update/delete methods call send() with ResponseStatus = SUCCESS, Response = null by default? Subclasses would only need to override the methods they care about.
2. What's the proper way to handle the situation when communication to the custom resource fails? Invoke some other method that subclasses could override if they wanted to?
3. Should this class itself implement RequestHandler?
A custom Response type to model the response data included in the send payload, plus a nested Builder for that type.

I'm debating between having Response hold name/value pairs vs. essentially be a wrapper for some java.lang.Object with option to customize its ObjectMapper. I know for my application's case, it's more convenient to manually convert my response object to a Map than configuring a custom ObjectMapper.

void sendResponse(event, context, status, CreateAppInstanceResponse chimeResponse) {
    // easier to do this than configure a mapper to serialize artifact object type
    Map<String, String> data = Map.of("AppInstanceArn", chimeResponse.appInstanceArn());
    new CloudFormationResponse(ApacheHttpClient.create()).send(event, context, status, data);
}

pankajagrawal16 · 2021-10-04T07:54:18Z

Should we have the create/update/delete methods call send() with ResponseStatus = SUCCESS, Response = null by default? Subclasses would only need to override the methods they care about.

Idea was to basically let users return a custom response object which we provide with builders etc. Users can decide based on the processing what will be the status etc. But basically we accept most of the parameters (Some optional probably) which users would want to send in response. This will include a custom object as well as part of response. The response object can then as well provide a method to accept custom object mapper which they would probably want to use when serializing response object. Builder dont need to then worry about sending the repsonse itself. That will be taken care by the handler which we implement as part of CloudformationCustomResource . Makes sense ?

pankajagrawal16 · 2021-10-04T07:55:24Z

What's the proper way to handle the situation when communication to the custom resource fails? Invoke some other method that subclasses could override if they wanted to?

It might be an idea to give an optional hook which is called in case send response invocation fails. Although I wonder what would user do in that case ? But still it might be a good idea to provide that flexibility .

pankajagrawal16 · 2021-10-04T07:56:17Z

Should this class itself implement RequestHandler?

That was my thought yes. So Builder just need to provide implementation of three methods and return the response object.

pankajagrawal16 · 2021-10-04T07:57:55Z

I'm debating between having Response hold name/value pairs vs. essentially be a wrapper for some java.lang.Object with option to customize its ObjectMapper. I know for my application's case, it's more convenient to manually convert my response object to a Map than configuring a custom ObjectMapper.

I believe it can accept any object, which could be a map as well if they want it to be. We can document that by default it will be serialized using default instance of object mapper, but if they want to use specific object mapper configuration, they can provide one as part of the custom Response object.

pankajagrawal16 · 2021-10-04T10:04:17Z

Is the SpotBugs / codecheck failure a false positive?
The logs do not show any violations for the impacted module: /home/runner/work/aws-lambda-powertools-java/aws-lambda-powertools-java/powertools-cloudformation/target/spotbugsXml.xml has 0 violations

Yeah I have been trying to figure this one out. It looks like it fails on PRs where author donot have access to repo. I need to find some time to troubleshoot this further.

Try rebasing with master and that should now fix spotbugs check issue for now.

bdkosher · 2021-10-06T00:43:45Z

Pushed some changes for your consideration.

I added the abstract resource handler and Response classes (with Builder) as we discussed, as well as a ResponseException (I wasn't feeling particularly creative with the name so I'm open to alternatives). The CloudFormationResponse is still there. It had been named to mimic the "cfn-response" module, but now with these news classes I'm thinking we should rename it to ResponseSender or something along those lines.

ResponseException is used to differentiate between an Exception occurring while constructing a response (ResponseException) vs. sending a response (IOException). Constructing the response can fail in two ways:

There's a RuntimeException thrown by the create/update/delete method of the handler
There's some RuntimeException/JacksonException thrown while attempting to merge the ResponseBody wrapper object with the Response object that ends up as the "Data" property.

I'm not fond of the the nested try/catches being used to handle these two failure scenarios--maybe there's a way to refactor CloudFormationResponse to just deal with "response sending" concerns and put the "response construction concerns" elsewhere that could be handled by a single try/catch.

If there's an exception while sending the response, I'm currently just logging it and invoking the empty-by-default onSendFailure method on the handler--there's not much you can really do at that point, as far as I know.

A final note, I'm still requiring the SdkHttpClient to be injected. I figured we could review these changes first and later figure out which HTTP client impl we wanted to use by default.

pankajagrawal16 · 2021-10-11T08:26:36Z

...ain/java/software/amazon/lambda/powertools/cloudformation/AbstractCustomResourceHandler.java

+        try {
+            response = getResponse(event, context);
+            LOG.debug("Preparing to send response {} to {}.", response, responseUrl);
+            client.send(event, context, ResponseStatus.SUCCESS, response);


May be its not rite to assume that user will always like to send status as Success ? Probably Response object should accept status as a field too and defaulting it to success ?

I like this. We could also even allow the physical resource ID and noEcho flag to be specified in the Response. Otherwise, with the way the handler is set up, there's no way to customize these values outside of invoking CloudFormationResponse::send directly from a custom handler.

Will still send success if the entire response is null; returning null from the create/update/delete methods is basically saying "I don't care about these type of of events".

pankajagrawal16 · 2021-10-11T08:28:08Z

...n/src/main/java/software/amazon/lambda/powertools/cloudformation/CloudFormationResponse.java

+ * <p>
+ * This class is thread-safe provided the SdkHttpClient instance used is also thread-safe.
+ */
+public class CloudFormationResponse {


Most of the implmentation here should now be internal ? and not be public as users only need to care about providing implementation for the abstract class ?

I'm OK with that. I had left it public in case anyone wanted to write their own custom handler, but given the nature of this code, I can't come up with a good reason why anyone would actually do that.

I still believe it's better to close it down. See it's easier to open it up based on community feedback and hearing use cases around it later than starting with an open one. Then its impossible to close it ever.

...mation/src/main/java/software/amazon/lambda/powertools/cloudformation/ResponseException.java

...java/software/amazon/lambda/powertools/cloudformation/AbstractCustomResourceHandlerTest.java

pankajagrawal16 · 2021-10-11T08:33:34Z

...java/software/amazon/lambda/powertools/cloudformation/AbstractCustomResourceHandlerTest.java

+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class AbstractCustomResourceHandlerTest {


Would the test be simpler to just verify and argument captor on mocked http client calls and arguments ?

Good call. Verifying the call to SdkHttpClient::prepareRequest is tricky because the HttpExecuteRequest arg doesn't expose readily-usable methods for validating things, but using Mockito to verify calls to create/update/delete on the handler itself would be more straightforward that manually counting invocations.

pankajagrawal16 · 2021-10-11T08:35:55Z

Pushed some changes for your consideration.

I added the abstract resource handler and Response classes (with Builder) as we discussed, as well as a ResponseException (I wasn't feeling particularly creative with the name so I'm open to alternatives). The CloudFormationResponse is still there. It had been named to mimic the "cfn-response" module, but now with these news classes I'm thinking we should rename it to ResponseSender or something along those lines.

ResponseException is used to differentiate between an Exception occurring while constructing a response (ResponseException) vs. sending a response (IOException). Constructing the response can fail in two ways:

There's a RuntimeException thrown by the create/update/delete method of the handler

There's some RuntimeException/JacksonException thrown while attempting to merge the ResponseBody wrapper object with the Response object that ends up as the "Data" property.

I'm not fond of the the nested try/catches being used to handle these two failure scenarios--maybe there's a way to refactor CloudFormationResponse to just deal with "response sending" concerns and put the "response construction concerns" elsewhere that could be handled by a single try/catch.

If there's an exception while sending the response, I'm currently just logging it and invoking the empty-by-default onSendFailure method on the handler--there's not much you can really do at that point, as far as I know.

A final note, I'm still requiring the SdkHttpClient to be injected. I figured we could review these changes first and later figure out which HTTP client impl we wanted to use by default.

This is great. Already starting to look nice. Left few suggestions, see if you agree with them.

bdkosher · 2021-10-11T18:32:49Z

Pushed another commit taking most of your feedback into consideration.

I left CloudFormationResource public for the time being. Since other language modules effectively provide that object alone, I was hesitant to make it inaccessible for Java classes.
The abstract Handler class still requires an SdkHttpClient arg at present. Still unsure about adding a default impl and dependency.
No documentation updates included as of yet. I plan to add them to the PR once we finalize the APIs.

pankajagrawal16 · 2021-10-14T04:28:32Z

Pushed another commit taking most of your feedback into consideration.

I left CloudFormationResource public for the time being. Since other language modules effectively provide that object alone, I was hesitant to make it inaccessible for Java classes.

The abstract Handler class still requires an SdkHttpClient arg at present. Still unsure about adding a default impl and dependency.

No documentation updates included as of yet. I plan to add them to the PR once we finalize the APIs.

I think everything looks much better. This is seems to shape up real nice now.

I still believe it's better to close CloudFormationResource down. See it's easier to open it up based on community feedback and hearing use cases around it later than starting with an open one. Then it's impossible to close it ever.

For Default client, I think especially for the use case of Custom resource, Cold start is not a big problem, so if I have to choose taking an extra dep over better builder experience and ease of use, I would say latter wins :)

pankajagrawal16 · 2021-10-14T04:31:17Z

In addition, I believe you also need to do changes here https://github.com/awslabs/aws-lambda-powertools-java/blob/master/.github/workflows/build.yml and few other files in .github folder for automation to work correctly for this module.

bdkosher · 2021-10-15T12:57:47Z

I still believe it's better to close CloudFormationResource down. See it's easier to open it up based on community feedback and hearing use cases around it later than starting with an open one. Then it's impossible to close it ever.

Makes sense. Will update it to be package-private.

For Default client, I think especially for the use case of Custom resource, Cold start is not a big problem, so if I have to choose taking an extra dep over better builder experience and ease of use, I would say latter wins :)

True. OK with "software.amazon.awssdk:url-connection-client" as the default client? Looks to be the smallest client size-wise.

you also need to do changes here https://github.com/awslabs/aws-lambda-powertools-java/blob/master/.github/workflows/build.yml

OK. Looks like that file and spotbugs.yml.

Also, do you see value in making 3 separate handler sub-classes each specific to one operation, e.g. abstract class CreateCustomResourceHandler extends AbstractCustomResourceHandler would implement the update and delete methods to return null; the developer would only need to provide the create method impl.

pankajagrawal16 · 2021-10-15T13:10:07Z

Also, do you see value in making 3 separate handler sub-classes each specific to one operation, e.g. abstract class CreateCustomResourceHandler extends AbstractCustomResourceHandler would implement the update and delete methods to return null; the developer would only need to provide the create method impl.

I would avoid doing this, One reason is to make it visible to customer that there are 3 parts to the puzzle of implementing a custom resource, so that its rite there for them to decide if they want to do anything on create or update or delete or not. Other is, Even though it will ease use and make code cleaner for ones who dont need it and know about it, I would avoid adding it until i see community ask as of now.

pankajagrawal16 · 2021-10-15T13:11:15Z

True. OK with "software.amazon.awssdk:url-connection-client" as the default client? Looks to be the smallest client size-wise.

Sounds good

bdkosher · 2021-10-18T12:37:00Z

Pushed another commit with the .github config, default SdkClient, lowered CloudFormationResource visibility, and a spotbugs fix. Provided things look good to you and the automated checks pass, I'll start the documentation.

pankajagrawal16 · 2021-10-18T12:38:38Z

Pushed another commit with the .github config, default SdkClient, lowered CloudFormationResource visibility, and a spotbugs fix. Provided things look good to you and the automated checks pass, I'll start the documentation.

Looks good to me. Probably we can start with documentation :)

docs/utilities/custom_resources.md

pankajagrawal16 · 2021-10-22T08:15:43Z

Missing entry of docs here https://github.com/awslabs/aws-lambda-powertools-java/blob/master/mkdocs.yml#L15

pankajagrawal16 · 2021-10-22T08:18:09Z

Looked through the docs and looks good.

bdkosher · 2021-10-22T14:35:26Z

Thanks for all the feedback. I'll push this missing commits from master to remove the pom.xml conflict.

Introduces Java implementation of cfn-response module based on NodeJS design. Put the sole class in a separate powertools-cloudformation Maven module. 100% code coverage Addresses aws-powertools#558

Provides abstract methods for generating the Response to be sent, represented as its own type. Use Objects::nonNull instead of custom method. Addresses aws-powertools#558

…r itself Instead of method args to various CloudFormationResponse::send methods, reducing the number of polymorphic send methods to two: one with a Response arg and one without. Rename ResponseException to CustomResourceResponseException. AbstractCustomResourceHandlerTest simplifications.

- Include powertools-cloudformation in .github config - Address mutatable ObjectMapper spotbugs finding - JavaDoc cleanup

pankajagrawal16

Looks good to go. Thanks for all the efforts @bdkosher 🙌

pull-request-size bot added the size/XL label Sep 28, 2021

pankajagrawal16 reviewed Oct 1, 2021

View reviewed changes

powertools-cloudformation/pom.xml Show resolved Hide resolved

pankajagrawal16 reviewed Oct 1, 2021

View reviewed changes

...n/src/main/java/software/amazon/lambda/powertools/cloudformation/CloudFormationResponse.java Outdated Show resolved Hide resolved

pankajagrawal16 reviewed Oct 1, 2021

View reviewed changes

bdkosher force-pushed the cfn_response branch from 8b5b0e2 to 7abe871 Compare October 6, 2021 00:22

pull-request-size bot added size/XXL and removed size/XL labels Oct 6, 2021

pankajagrawal16 reviewed Oct 11, 2021

View reviewed changes

...mation/src/main/java/software/amazon/lambda/powertools/cloudformation/ResponseException.java Outdated Show resolved Hide resolved

pankajagrawal16 reviewed Oct 11, 2021

View reviewed changes

...java/software/amazon/lambda/powertools/cloudformation/AbstractCustomResourceHandlerTest.java Outdated Show resolved Hide resolved

pankajagrawal16 reviewed Oct 11, 2021

View reviewed changes

bdkosher force-pushed the cfn_response branch from 0c612b6 to 9e09e7d Compare October 18, 2021 12:33

msailes reviewed Oct 20, 2021

View reviewed changes

docs/utilities/custom_resources.md Outdated Show resolved Hide resolved

msailes reviewed Oct 20, 2021

View reviewed changes

docs/utilities/custom_resources.md Outdated Show resolved Hide resolved

msailes reviewed Oct 20, 2021

View reviewed changes

docs/utilities/custom_resources.md Outdated Show resolved Hide resolved

msailes reviewed Oct 20, 2021

View reviewed changes

docs/utilities/custom_resources.md Outdated Show resolved Hide resolved

Joe Wolf added 6 commits October 22, 2021 11:30

Create Java cfn-response equivalent (aws-powertools#558)

2f1636a

Introduces Java implementation of cfn-response module based on NodeJS design. Put the sole class in a separate powertools-cloudformation Maven module. 100% code coverage Addresses aws-powertools#558

Add base RequestHandler class for custom resources

5e835ae

Provides abstract methods for generating the Response to be sent, represented as its own type. Use Objects::nonNull instead of custom method. Addresses aws-powertools#558

Lock down CloudFormationResponse; default SdkHttpClient

4b9e7c0

- Include powertools-cloudformation in .github config - Address mutatable ObjectMapper spotbugs finding - JavaDoc cleanup

Cloudformation module documentation

043afc5

CloudFormation documentation tweaks

3627d4d

bdkosher force-pushed the cfn_response branch from 15af6f9 to 3627d4d Compare October 22, 2021 15:35

Include custom resources doc in menu

89686ef

pankajagrawal16 approved these changes Oct 22, 2021

View reviewed changes

pankajagrawal16 changed the title ~~Create Java cfn-response equivalent (#558)~~ feat: create Java cfn-response equivalent (#558) Oct 24, 2021

pankajagrawal16 merged commit 35ad5f6 into aws-powertools:master Oct 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create Java cfn-response equivalent (#558) #560

feat: create Java cfn-response equivalent (#558) #560

bdkosher commented Sep 28, 2021 •

edited by pankajagrawal16

Loading

bdkosher commented Sep 28, 2021

msailes commented Sep 30, 2021

bdkosher commented Sep 30, 2021

pankajagrawal16 commented Oct 1, 2021

pankajagrawal16 Oct 1, 2021

bdkosher Oct 1, 2021

pankajagrawal16 commented Oct 1, 2021 •

edited

Loading

bdkosher commented Oct 1, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

bdkosher commented Oct 6, 2021

pankajagrawal16 Oct 11, 2021

bdkosher Oct 11, 2021 •

edited

Loading

pankajagrawal16 Oct 11, 2021

bdkosher Oct 11, 2021

pankajagrawal16 Oct 14, 2021

pankajagrawal16 Oct 11, 2021

bdkosher Oct 11, 2021

pankajagrawal16 commented Oct 11, 2021

bdkosher commented Oct 11, 2021

pankajagrawal16 commented Oct 14, 2021 •

edited

Loading

pankajagrawal16 commented Oct 14, 2021

bdkosher commented Oct 15, 2021

pankajagrawal16 commented Oct 15, 2021 •

edited

Loading

pankajagrawal16 commented Oct 15, 2021

bdkosher commented Oct 18, 2021

pankajagrawal16 commented Oct 18, 2021

pankajagrawal16 commented Oct 22, 2021

pankajagrawal16 commented Oct 22, 2021

bdkosher commented Oct 22, 2021

pankajagrawal16 left a comment

feat: create Java cfn-response equivalent (#558) #560

feat: create Java cfn-response equivalent (#558) #560

Conversation

bdkosher commented Sep 28, 2021 • edited by pankajagrawal16 Loading

Description of changes:

Breaking change checklist

bdkosher commented Sep 28, 2021

msailes commented Sep 30, 2021

bdkosher commented Sep 30, 2021

pankajagrawal16 commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pankajagrawal16 commented Oct 1, 2021 • edited Loading

bdkosher commented Oct 1, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

pankajagrawal16 commented Oct 4, 2021

bdkosher commented Oct 6, 2021

Choose a reason for hiding this comment

bdkosher Oct 11, 2021 • 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

pankajagrawal16 commented Oct 11, 2021

bdkosher commented Oct 11, 2021

pankajagrawal16 commented Oct 14, 2021 • edited Loading

pankajagrawal16 commented Oct 14, 2021

bdkosher commented Oct 15, 2021

pankajagrawal16 commented Oct 15, 2021 • edited Loading

pankajagrawal16 commented Oct 15, 2021

bdkosher commented Oct 18, 2021

pankajagrawal16 commented Oct 18, 2021

pankajagrawal16 commented Oct 22, 2021

pankajagrawal16 commented Oct 22, 2021

bdkosher commented Oct 22, 2021

pankajagrawal16 left a comment

Choose a reason for hiding this comment

bdkosher commented Sep 28, 2021 •

edited by pankajagrawal16

Loading

pankajagrawal16 commented Oct 1, 2021 •

edited

Loading

bdkosher Oct 11, 2021 •

edited

Loading

pankajagrawal16 commented Oct 14, 2021 •

edited

Loading

pankajagrawal16 commented Oct 15, 2021 •

edited

Loading