make smoke tests Java 8 compatible #3187
Conversation
| ); | ||
|
|
||
| write(""" | ||
| file(GLOB AWS_$L_GENERATED_SMOKE_TEST_SRC |
There was a problem hiding this comment.
Multi line strings not supported on Java 8. Hence needed refactoring
| if (services.size != 1) { | ||
| throw Exception("There must be exactly one service in each aws model file, but found " + | ||
| "${services.size} in ${file.name}: ${services.map { it.id }}"); | ||
| println("Unexpected number of service shapes detected: ${services.size} while processing file: ${file.name}. ") |
There was a problem hiding this comment.
deliberately not throwing exception to allow other services to move on
There was a problem hiding this comment.
we should catch and throw issues when we find them, and leave them up to the caller, i.e. the CI script to ignore. for instance here, why are there more than 1 service shape? what is wrong with the smithy files that allows this to be the case?
There was a problem hiding this comment.
yes, but it affects all the other services that don't have issue. I can still throw it but it is going to show incomplete builds in pipeline if even one service has an issue
There was a problem hiding this comment.
the doesn't have anything to do with the pipeline, or CI at all, at pure level, if the code generator fails, it should bubble up the error to the caller, and the caller is responsible for handling it. in this case the CI pipeline.
for instance here, why are there more than 1 service shape? what is wrong with the smithy files that allows this to be the case?
you didnt answer this question -- why do we need this and is this a data problem. what services cause this problem and why.
.../main/java/com/amazonaws/util/awsclientsmithygenerator/generators/SmithyC2JNamespaceMap.java
Show resolved
Hide resolved
| "aws/testing/AwsCppSdkGTestSuite.h", | ||
| "aws/testing/AwsTestHelpers.h" | ||
| ); | ||
| this.unitTestHeaders = new HashSet<>(); |
There was a problem hiding this comment.
Should use guavas ImmubtableSet.of and add the guava dep if needed, would be nice but not required
|
|
||
| dynamicHeaders.add(String.format("aws/%s/%sClient.h", c2jNamespace, clientNamespace)); | ||
| //this will be added to based upon datastructures found | ||
| containerHeaderMap = new HashMap<>(); |
There was a problem hiding this comment.
Similar as above should use guavas 'ImmutableMap.of' and add the dep
|
|
||
| package com.amazonaws.util.awsclientsmithygenerator.generators; | ||
|
|
||
| import lombok.val; |
There was a problem hiding this comment.
Dont use Lombok val, spell out the complete type
There was a problem hiding this comment.
ok, that was IDE suggestion. will change
.../main/java/com/amazonaws/util/awsclientsmithygenerator/generators/SmithyC2JNamespaceMap.java
Show resolved
Hide resolved
| if (services.size != 1) { | ||
| throw Exception("There must be exactly one service in each aws model file, but found " + | ||
| "${services.size} in ${file.name}: ${services.map { it.id }}"); | ||
| println("Unexpected number of service shapes detected: ${services.size} while processing file: ${file.name}. ") |
There was a problem hiding this comment.
we should catch and throw issues when we find them, and leave them up to the caller, i.e. the CI script to ignore. for instance here, why are there more than 1 service shape? what is wrong with the smithy files that allows this to be the case?
3 participants
Issue #, if available:
*Description of changes:
Previous iteration of changes are based on Java 16 which is not compatible with cdk. Hence, this change is necessary.
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.