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
XAWS-4: Added CDK code for XWiki demo installation with unit tests. #6
XAWS-4: Added CDK code for XWiki demo installation with unit tests. #6
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.
Hi @sanchita141011,
The code styling seems a bit inconsistent, could you maybe set up prettier and ideally eslint as well? It would make it easier to maintain code checking and styling for the future. ;)
Thankyou for suggestion. :) |
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.
Looks good to me (TypeScript-wise — I don't know much about CDK). ;)
Thanks for the changes.
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.
Add better tests. Remove import * statements. Add entry point file to this cdk app. Currently, there's only single stack with no entry point for this application. This code won't work.
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA | ||
* 02110-1301 USA, or see the FSF site: http://www.fsf.org. | ||
*/ | ||
import * as cdk from '@aws-cdk/core' |
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.
Remove all * imports. Import only the modules you want in the file.
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.
Done :)
* 02110-1301 USA, or see the FSF site: http://www.fsf.org. | ||
*/ | ||
import * as cdk from '@aws-cdk/core' | ||
import * as ec2 from '@aws-cdk/aws-ec2' |
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.
Remove all * imports. Import only the modules you want in the file.
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.
Done :)
*/ | ||
import * as cdk from '@aws-cdk/core' | ||
import * as ec2 from '@aws-cdk/aws-ec2' | ||
import * as iam from '@aws-cdk/aws-iam' |
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.
Remove all * imports. Import only the modules you want in the file.
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.
Done :)
import * as fs from 'fs' | ||
|
||
export class EC2XwikiDemo extends cdk.Stack { | ||
constructor (scope: cdk.Construct, id: string, props?: cdk.StackProps) { |
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.
Scope here should be cdp.App and not cdk.Construct
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.
corrected :)
constructor (scope: cdk.Construct, id: string, props?: cdk.StackProps) { | ||
super(scope, id, props) | ||
|
||
const defaultVpc = ec2.Vpc.fromLookup(this, 'VPC', { isDefault: true }) |
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.
Don't hard code VPC. In case default vpc is not present , then the code will fail. Take it from the consumer. Give him the flexibility to choose default vpc or create a new one or pass any other vpc. Manage it in entry point of the cdk application.
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.
Changed the VPC from default to new VPC.
import * as cdk from '@aws-cdk/core'; | ||
import * as XwikiDemoCdk from '../lib/xwiki-demo-cdk-stack'; | ||
import { expect as expectCDK, haveResourceLike, countResources } from '@aws-cdk/assert' | ||
import * as cdk from '@aws-cdk/core' |
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.
Dont use import * statements.
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.
Done
|
||
// expect(stack).toBe(countResources("AWS::EC2::Instance", 1)); | ||
// THEN | ||
expectCDK(stack).to( |
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.
Add snapshot tests for CDK.
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.
Added snapshot test
// THEN | ||
expectCDK(stack).to( | ||
countResources('AWS::EC2::Instance', 1)) | ||
|
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.
dd more granular tests. Just testing the presence of ec2 instance is not gonna make it. Check for other resources you created via CDK too.
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.
added unit test to check EC2 instance, VPC, IAM Role, Security Group and to check if EC2 instance is launched inside a subnet with public IPv4. Please check and suggest changes If required
…and to check if EC2 instance is launched inside a subnet with public IPv4
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.
Add more granular tests is possible.
xwiki-demo-cdk/Bin/xwiki-demo-cdk.ts
Outdated
new EC2XwikiDemo(app, 'ec2XwikiDemo', { | ||
xwiki: xwikidownload, | ||
env: { | ||
region: 'eu-central-1' |
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.
Dont hard code the region. Get it from some configuration file as discussed
@@ -0,0 +1,7 @@ | |||
import { StackProps } from '@aws-cdk/core' | |||
|
|||
export interface ec2props extends StackProps{ |
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.
Add JavaDoc for all classes. Explain parameters in comments. What's the use of each.
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.
Done. Please suggest the changes if required
|
||
}) | ||
|
||
// const commands = ['sudo yum -q -y install java-1.8.0-openjdk && sudo yum -q -y install unzip && mkdir xwikihome && wget -q -O xwiki_packer.zip https://nexus.xwiki.org/nexus/content/groups/public/org/xwiki/platform/xwiki-platform-distribution-flavor-jetty-hsqldb/13.1/xwiki-platform-distribution-flavor-jetty-hsqldb-13.1.zip && unzip -q xwiki_packer.zip -d /home/ec2-user/xwikihome'] |
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.
Remove these lines if they are not needed.
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.
Removed
sudo yum -q -y install java-1.8.0-openjdk | ||
sudo yum -q -y install unzip | ||
mkdir xwikihome | ||
wget -q -O xwiki_packer.zip https://nexus.xwiki.org/nexus/content/groups/public/org/xwiki/platform/xwiki-platform-distribution-flavor-jetty-hsqldb/13.1/xwiki-platform-distribution-flavor-jetty-hsqldb-13.1.zip |
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.
Remove this file.
Good work on the PR. Hope to get the Production PR merged too soon after it's completed. |
Added CDK code for installing XWiki demo using user-script on EC2 instance of type T2.medium inside public subnet of default VPC of the user's account. Added unit tests using jest test framework.