fix(aws-cdk): Move version check TTL file to home directory #2774
Conversation
Originally, the version check TTL file was located alongside the CDK node installation directory. This caused issues for people who had CDK installed on a readonly filesystem. This change moves this file to the user's home directory, specifically under $HOMEDIR/.cdk/cache. This directory is already being used by account-cache.ts. The old logic is maintained where a repo check is run once a day against NPM, when a CDK command is invoked, and when a new version is present will generate a console banner notifying the user of this. The edge case with the updated implementation is when there are 2+ active installations of CDK and they are used interchangably. When this happens, the user will only get one notification per non-latest installation of CDK per day.
|
Can someone from the @awslabs/aws-cdk-team either provide access to the failing CodeBuild or provide the details on what commands are run on the CodeBuild fleet along with the output somewhere? The code related to my commit and their associated unit tests seems to run fine on my laptop. However, I have a failing test |
|
| mkdirsSync(path.dirname(this.file)); | ||
| fs.accessSync(path.dirname(this.file), fs.constants.W_OK); | ||
| } catch { | ||
| throw new Error(`Directory (${path.dirname(this.file)}) is not writable.`); |
There was a problem hiding this comment.
Is this worth throwing? Shouldn't we flip a boolean that says "can't do any work, soz" ?
There was a problem hiding this comment.
The "can't do any work" message is printed by the catch block at the calling point of this constructor.
I did consider flipping a boolean and keeping it as a class member. This would mean the other methods on the class (like hasExpired()) return an additional state to the caller, which the caller will need to take care of.
Throwing an exception here seemed simpler and achieves the same experience.
* Drop use of fs and use fs-extra exclusively. * Cleaned up file write. * Fixed breaking unit test. #2774
There was a problem hiding this comment.
Sorry for chiming in late in the game... one piece of feedback: I want to make sure this will never fail or block the user or even show an error message in case it couldn’t reach the network. The only situation where it should emit any output should be when there is a new version to be installed. In all other cases, it should be completely hidden from the user experience.
(I got some nasty error messages when I was in the plane).
|
@eladb - Thanks for that. I did some digging here and I think I know what caused this for you. If there is no internet connectivity *and* npm has not cached any previous version of There's one log message at |
Motivation & Change
Originally, the version check TTL file was located alongside the CDK
node installation directory. This caused issues for people who had CDK
installed on a readonly filesystem.
This change moves this file to the user's home directory, specifically
under
$HOMEDIR/.cdk/cache. This directory is already being used byaccount-cache.ts.The old logic is maintained where a repo check is run once a day against
NPM, when a CDK command is invoked, and when a new version is present
will generate a console banner notifying the user of this.
The edge case with the updated implementation is when there are 2+
active installations of CDK and they are used interchangably. When
this happens, the user will only get one notification per non-latest
installation of CDK per day.
Manual Testing
Unit tests exist but beyond that, manual checks run using the command
cdk init --listPull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.