-
Notifications
You must be signed in to change notification settings - Fork 27
feat: record CLI version in manifest package #184
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
Conversation
Record the current CLI version in the manifest package when we release the manifest. This will have the effect of recording the CLI version that supports a given manifest version.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
- Coverage 85.36% 85.09% -0.27%
==========================================
Files 207 207
Lines 35816 35837 +21
Branches 4663 4625 -38
==========================================
- Hits 30575 30497 -78
- Misses 5084 5190 +106
+ Partials 157 150 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
projenrc/insert-task-step.ts
Outdated
const releaseTask = this.project.tasks.tryFind(this.props.taskName); | ||
if (!releaseTask) { | ||
throw new Error(`Did not find task ${this.props.taskName}`); | ||
} | ||
|
||
// Find the bump task, and do the CLI version copy straight after | ||
const bumpIx = releaseTask.steps.findIndex(s => s.exec === this.props.beforeExec); | ||
if (bumpIx === -1) { | ||
throw new Error(`Did not find step: ${this.props.beforeExec}`); | ||
} | ||
|
||
releaseTask.steps.splice(bumpIx, 0, ...this.props.insertSteps); |
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.
Variable names here are not reflecting the generic task
.projenrc.ts
Outdated
insertSteps: [ | ||
{ exec: 'ts-node projenrc/copy-cli-version-to-assembly.task.ts' }, | ||
], | ||
beforeExec: 'yarn workspaces run build', |
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 suggest making the necessary changes to InsertTaskStep
so that invoking it eventually looks like this:
beforeExec: 'yarn workspaces run build', | |
afterExec: 'yarn workspaces run bump', |
Since the bump
task is our "dependency" here.
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 had that first but I find a "before" much more natural for an "insert" operation.
Also, yes it has to be after bump, but it also equally has to be before build, so both make sense.
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 had that first but I find a "before" much more natural for an "insert" operation.
hh so call it AppendTaskStep
:)
Its fine. just a personal preference.
.projenrc.ts
Outdated
@@ -374,11 +375,24 @@ new JsiiBuild(cloudAssemblySchema, { | |||
(() => { | |||
cloudAssemblySchema.preCompileTask.exec('tsx projenrc/update.ts'); | |||
|
|||
// This file will be generated at release time. It needs to be gitignored or it will | |||
// fail projen's "no tamper" check, which means it must also be generated every build time. | |||
cloudAssemblySchema.preCompileTask.exec('[[ -f cli-version.json ]] || echo \'{ "version": "" }\' > cli-version.json'); |
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.
Can we not call the same copy-cli-version-to-assembly.task.ts
script and have it create the empty file in case the version is 0.0.0? Just trying to minimize bash.
@@ -0,0 +1,24 @@ | |||
import { promises as fs } from 'fs'; |
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.
just FYI, not sure which is better
import { promises as fs } from 'fs'; | |
import * fs from 'fs/promises'; |
Signed-off-by: github-actions <[email protected]>
Signed-off-by: github-actions <[email protected]>
### Issue # (if applicable) N/A ### Reason for this change This upgrade is supposed to be done as part of #34362. But the `@aws-cdk/cloud-assembly-schema` upgrade requires a manual update to the unit test so I am isolating this upgrade to this PR. `v41.2.0` introduced a new line in the manifest file (PR: aws/aws-cdk-cli#184), hence, the unit test was failing. ### Description of changes Upgraded `@aws-cdk/cloud-assembly-schema` to `v41.2.0` and updated unit test. ### Describe any new or updated permissions being added None ### Description of how you validated changes Ran the build locally and no longer see the unit test failing ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Record the current CLI version in the manifest package when we release the manifest.
This by itself will not trigger a new release of the
cloud-assembly-schema
package, so the version only gets recorded (and published!) when there is a new version ofcloud-assembly-schema
to publish anyway, and the version will only be displayed back to the user if there is an incompatibility of the major version number.That means we won't necessarily show the lowest possible CLI version number when the user needs to upgrade, but we'll show a valid CLI version number that will definitely support the given manifest.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license