-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Added the Java backend for weathertop2 #7604
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,58 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File name has a typo:
WeatheetopTest.java should be WeatherTopTest.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this file name to remove typo
| - **Detailed Reports**: Generates a Dashboard in client app to inform you of the details such as pass rate for the SDKs, the overall number of tests, and so on. | ||
|
|
||
| ## Getting Started | ||
| ToDO No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide necessary info under Getting Started
| FargateTaskRunner runner = new FargateTaskRunner(); | ||
| String taskRunId = ""; | ||
| if (language.compareTo("java") == 0) { | ||
| String clusteerName = "MyJavaWeathertopCluster"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo; should be clusterName instead of clusteerName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same typo is also at the next 6 else if conditions.
| private static final Region REGION = Region.US_EAST_1; | ||
|
|
||
| // Network config matching Python exactly | ||
| private static final List<String> SUBNETS = List.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it required to hard code the subnet and security group config?
| @@ -0,0 +1,58 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this file name to remove typo
rlhagerm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments and questions about the code.
Also, where is the code that writes the test outputs to S3? Is that in updated docker files, or somewhere else?
| - **Detailed Reports**: Generates a Dashboard in client app to inform you of the details such as pass rate for the SDKs, the overall number of tests, and so on. | ||
|
|
||
| ## Getting Started | ||
| ToDO No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see more detail here, such as a section on deployment, and one on logging. This is a place to describe how to work with the code, not just the overview and features.
| "ecs-java-schedule" | ||
| ); | ||
|
|
||
| Map<String, Object> data = inspector.inspect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 12 lines of code are repeated 10 times, can they be consolidated by using variables within the switch and then the inspector/json/response return all in one place at the end to make it more maintainable?
| logger.log("Processing Java SDK EventBridge Inspector"); | ||
|
|
||
| EcsEventBridgeInspector inspector = new EcsEventBridgeInspector( | ||
| "MyJavaWeathertopCluster", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all of these hardcoded resource names, is there a deployment stack somewhere to ensure they are deployed correctly with all the right names?
| } | ||
|
|
||
| String taskDefinitionArnVal = queryParams.get("taskDefinitionArnVal").toLowerCase(); // Normalize | ||
| String clusterName = queryParams.get("clusterName").toLowerCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable clusterName is forced to lower case, but then it is compared to values that are not all lower case, like "MyNetWeathertopCluster". How does this ever match?
| } else if (clusterName.equals("MyRustWeathertopCluster")) { | ||
| ruleName = "ecs-rust-schedule"; | ||
| } else { | ||
| ruleName = "ecs-java-schedule"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like enough languages. Where are PHP, CPP, Python, etc?
| private static final String AWS_ACCOUNT_ID = "814548047983"; | ||
| private static final String AWS_REGION = "us-east-1"; | ||
| private static final String CLUSTER_NAME = "MyJavaWeathertopCluster"; | ||
| private static final String TASK_DEF_NAME = "WeathertopJava"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this deploy some resources for Java? What about other languages? And where is this called?
| ensureLogGroup(); | ||
| String taskDefArn = registerTaskDefinition(); | ||
| createOrUpdateService(clusterArn, taskDefArn); | ||
| System.out.println("✅ ECS Fargate deployment complete."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emoji.
|
|
||
| public class FargateDeployer { | ||
|
|
||
| private static final String AWS_ACCOUNT_ID = "814548047983"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded account?
|
|
||
| if (!response.tasks().isEmpty()) { | ||
| String taskArn = response.tasks().get(0).taskArn(); | ||
| System.out.println("🚀 Started task: " + taskArn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check all code for emojis.
| * @return the key of the latest file that matches the pattern | ||
| * @throws RuntimeException if no matching files are found for the given prefix | ||
| */ | ||
| public static String getLatestFileKey(S3Client s3Client, String bucketName, String languagePrefix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there multiple copies of both getLatestFileKey and extractTimestamp? If they are common utilities, let's put them in a common location.
This pull request adds Java backend for weathertop
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.