-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat(core): make cloudwatch agent installation optional #338
Conversation
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 some small comments.
Can you elaborate on how this was tested?
THIRD-PARTY
Outdated
|
||
------ | ||
|
||
** openssl 1.0.2k-fips; version 1.0.2k-fips -- https://www.openssl.org/ |
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.
Ditto here for version number
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.
Updated.
host: IScriptHost, | ||
shouldInstallAgent: boolean, | ||
skipValidation: boolean, | ||
) { | ||
// Grant access to the required CloudWatch Agent installer files | ||
const cloudWatchAgentBucket = Bucket.fromBucketArn(this, 'CloudWatchAgentBucket', 'arn:aws:s3:::amazoncloudwatch-agent'); |
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 noticed that we missed updating this. The bucket ARN changes based on the region now, no?
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.
Yes good catch, this was definitely a miss in my testing. When running the script through the RFDK example app, I was using the Thinkbox Deadline Linux and Windows base AMI's, which both have the CloudWatch agent pre-installed.
Since I don't have an AMI with Deadline installed but the CloudWatch agent not installed, I commented out the check for an existing agent in both configuration scripts and then turned off the worker fleet's health check so I could deploy the fleets and confirm the failure by logging into the hosts through the Systems Manager and checking the logs. After that, I updated this code to grant permissions for the regional bucket and redeployed to confirm that it was working.
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 one small comment, otherwise this looks good to me!
Fixes #308
Testing was a bit tricky to go through all the different code paths. I extracted the powershell and bash scripts and modified them slightly to work on their own, then manually spun up Windows and Linux hosts through the EC2 console using the scripts as user data. Doing this I was able to make modifications like setting the inputs to different values to using bad S3 locations I knew it wouldn't be able to download from, to see how it worked through different code paths. After I could use the scripts successfully like that, I then deployed the Basic example app, modified to have both a Linux and Windows worker fleet, to make sure that the unmodified scripts worked.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license