Skip to content
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

This is an example using the AWS SDK for Go to list ec2 instances ins… #1481

Closed

Conversation

piusranjan
Copy link

This is an example using the AWS SDK for Go to list ec2 instances instance state by different region . By default it fetch all running and stopped instance

Usage go run filter_ec2_by_region running

…tance state By different region . By default it fetch all running and stopped instance
Copy link
Contributor

@xibz xibz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @piusranjan, thank you for taking the time to put this together. I have a few comments that I think is mostly for consistency.

}
} else {
States= []*string{
aws.String("running"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use constants here, like ec2.InstanceStateNameRunning

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would either get rid of this else statement or not pass in states


import (
"fmt"
"github.com/aws/aws-sdk-go/aws/session"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, but generally we put stdlib stuff separate from SDK imports

import (
    "fmt"
    "os"

    "github.com/aws/aws-sdk-go/aws/session"
    // and so on
)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xibz . Thank. I will change this.

)

func fetchRegion()[]string{

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the unneeded whitespace here for consistency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks , I will change this


regions := []string{}
sess1 := session.Must(session.NewSession(&aws.Config{
Region: aws.String("us-west-1"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Region shouldn't be hard coded, please pass this in via command line

}))

svc := ec2.New(sess1)
awsregions, err := svc.DescribeRegions(&ec2.DescribeRegionsInput{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace not properly aligned. Run go fmt to fix this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am new to go. I will take care.


svc := ec2.New(sess1)
awsregions, err := svc.DescribeRegions(&ec2.DescribeRegionsInput{})
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not want to return this error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I will change it. It should .

aws.String("running"),
aws.String("pending"),
aws.String("stopped"),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace not following Go standard

&ec2.Filter{
Name: aws.String("instance-state-name"),
Values: States,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

States:= []*string{}
if len(os.Args)>1 {
for i := 1; i < len(os.Args); i++ {
States=append(States,&os.Args[i])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

region should be passed in via command line as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed


Fetching instace details for region: ap-south-1
printing instance details.....
instance id i-00cf3fcsssdd373766
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably want some mock value here like x-000000000000000000

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xibz , what mock value you mean to say? Are you talking about any unit test with some mock value?

@piusranjan
Copy link
Author

@xibz . Thanks for all your valuable review . I will change all according to your sugesstion

@piusranjan
Copy link
Author

@xibz , I have made changes as per your suggestion. Please have a look.

@piusranjan
Copy link
Author

Not sure why the build is failing in github.com/aws/aws-sdk-go/service/s3/s3manager for go 1.6. How my code is related to that module?

@piusranjan
Copy link
Author

Closing because I will create a new PR to make a fresh build

@jasdel
Copy link
Contributor

jasdel commented Aug 25, 2017

Thanks for updating the CR @piusranjan I've taking your changes with minor tweaks to #1492. I'll close this PR and merge in the new one.

@jasdel jasdel closed this Aug 25, 2017
jasdel added a commit that referenced this pull request Aug 25, 2017
Adds an example using the AWS SDK for Go to list EC2 instances instance state by different region.

Minor tweak of #1481 for style and example consistence.
@piusranjan
Copy link
Author

@jasdel thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants