chore: add sync models task and sync latest models #374
Conversation
|
|
||
| pairs.forEach { (source, existing) -> | ||
| // ensure we don't accidentally take a new API version | ||
| if (source.version != existing.version) error("upstream version of ${source.path} does not match destination of existing model version ${existing.modelFile.name}") |
There was a problem hiding this comment.
Nit: This line is 176 chars long. Do we have any expectations about line length in Gradle scripts?
There was a problem hiding this comment.
No, but we could put it into braces such that the error is on a new line, e.g.:
if (...) {
error(...)
}Is that preferable?
There was a problem hiding this comment.
Also I'm not even sure if this check matters or not but I erred on the side of caution
There was a problem hiding this comment.
Yes, I generally prefer code with limited line length. It helps me keep multiple windows open in my IDE and reduces cognitive load when reading PRs.
| // models with no upstream source in aws-models | ||
| orphaned.forEach { sdkId -> | ||
| val existing = existingModelsBySdkId[sdkId]!! | ||
| logger.warn("Cannot find a model file for ${existing.modelFile.name} (sdkId=${existing.sdkId}) but it exists in aws-sdk-kotlin!") | ||
| } |
There was a problem hiding this comment.
Question: Should this be an error instead? I don't know how often this actually happens (vs how often this would indicate a bug). In either case, it seems like we'd want a human to definitively acknowledge it or take action (e.g., manually deleting the service from aws-sdk-kotlin).
There was a problem hiding this comment.
I wasn't sure...it hasn't happened yet (on the one time I have synced using this script 🤣 ). I can go either way. I had sort of wanted logger.warn to spit out colored output so that these log messages were more visible but I don't see any difference 🤷
There was a problem hiding this comment.
Warnings rely on good intent and careful diligence on the part of operators. Errors force us to make explicit, intentional decisions no matter how many other things are competing for our attention.
This means that there is a new workflow by which we locally pull the models repo, run the task, and submit a PR? Is that the workflow?
Did this new approach get used to sync latest models? |
codegen/sdk/build.gradle.kts
Outdated
| return getProperty("awsModelsDir")?.let { File(it) }?.absolutePath | ||
| } | ||
|
|
||
| tasks.create("syncModels") { |
There was a problem hiding this comment.
suggestion
as we already have various models, i think it would be better to qualify this task as you do in the println. syncAwsModels or something.
|
|
||
| // generate warnings at the end so they are more visible | ||
| if (orphaned.isNotEmpty() || newSources.isNotEmpty()) { | ||
| println("\nWarnings:") |
There was a problem hiding this comment.
question
why are we mixing println() and logger.xxx()...is it that info doesn't print by default or something?
There was a problem hiding this comment.
Yeah. I had hoped logger.warn did something more...visible 🤷 . They could all be println if you prefer
There was a problem hiding this comment.
it's nbd. something to add to consideration for gradle build cleanup. i think it would be a regression to use println, better to figure out how to make the logger work as we'd like it to.
|
|
||
|
|
||
| fun discoverAwsModelsRepoPath(): String? { | ||
| val discovered = rootProject.file("../aws-models") |
There was a problem hiding this comment.
concern
Unless I missed it in comments, I think we need a blurb somewhere that describes the steps to update models with this new code.
There was a problem hiding this comment.
Sure I can add something but in general
cd <path/to/local/aws-models>
git pull // ensure we are up to date with latest commits
cd <path/to/aws-sdk-kotlin>
./gradlew syncModels
|
Kudos, SonarCloud Quality Gate passed!
|
3 participants
Issue #
N/A
Description of changes
aws-sdk-go-v2) as the source for models. You can now pull down the source models repo and sync it directly.codegen/sdk/build.gradle.kts)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.