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

New ScheduledV2Event implementation #480

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

peniakoff
Copy link

and fix for APIGatewayV2CustomAuthorizerEventTest when running on the non-US locale machine.

Issue #, if available:

No. 479: #479

Description of changes:

New Java class ScheduledV2Event has been added. Additionally change is related to APIGatewayV2CustomAuthorizerEventTest which didn't pass on non-US locale Java machine. Now default locale for both tests from this class are running under US-en locale.

Target (OCI, Managed Runtime, both):

both

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

and fix for APIGatewayV2CustomAuthorizerEventTest when running on the non-US locale machine
Copy link
Contributor

@smirnoal smirnoal left a comment

Choose a reason for hiding this comment

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

please add a test to aws-lambda-java-tests covering this event

@@ -0,0 +1,366 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

please update the header to

/*
Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
SPDX-License-Identifier: Apache-2.0
*/

@@ -12,6 +14,11 @@ public class APIGatewayV2CustomAuthorizerEventTest {
private static final long TIME_EPOCH = 1601306426515L;
private static final String TIME = "28/Sep/2020:15:14:43 +0000";

@BeforeAll
static void beforeAll() {
Locale.setDefault(new Locale("en", "US"));
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for this

@peniakoff
Copy link
Author

@smirnoal, done with my last commit.

@smirnoal smirnoal requested review from msailes and andclt May 7, 2024 14:13
import java.util.List;

/**
* represents a scheduled V2 event
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a link to the documentation in the header please.


private String region;

private String detail;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we (AWS) needs to validate that there will be no events where detail will be anything other than a String for scheduled task use cases.

@msailes
Copy link
Collaborator

msailes commented Jun 24, 2024

We also probably want to consider how we explain the difference between ScheduledEvent and ScheduledV2Event. Are we deprecating ScheduledEvent?

@peniakoff
Copy link
Author

PR updated. Sorry for delay.

* @see Object#toString()
*/
@Override
public String toString() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://projectlombok.org/features/Data includes toString, hashcode and equals, you can remove those, and the clone method.

Copy link
Author

Choose a reason for hiding this comment

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

Done.


/**
* Represents a Scheduled V2 event sent to Lambda
* <a href="https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-property-function-schedulev2.html">ScheduleV2</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a link to the wrong page.

Copy link
Author

Choose a reason for hiding this comment

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

This one should be more valid. 😉

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