-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve AMI generator #273
Conversation
} | ||
|
||
// Sort on region | ||
sort.Slice(amiImages.General, func(i, j int) bool { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
pkg/ami/static_resolver_ami.go
Outdated
|
||
// Code generated by go generate; DO NOT EDIT. | ||
// Generated code |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
package ami | ||
ImageClassGPU: { | ||
|
||
"eu-west-1": "ami-0706dc8a5eed2eed9", |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
||
package ami | ||
ImageClassGPU: { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
be62de8
to
3c1ce9c
Compare
"github.com/weaveworks/eksctl/pkg/ami" | ||
"github.com/weaveworks/eksctl/pkg/eks/api" | ||
|
||
. "github.com/dave/jennifer/jen" |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Yeah, I just copied it from example code and felt like it seemed prettier
for this rather verbose library. And it is only a generator, so no big
deal...
…On Thu, 18 Oct 2018, 8:45 pm Bryan Peterson, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/ami/static_resolver_ami_generate.go
<#273 (comment)>:
> +
+package main
+
+import (
+ "fmt"
+ "log"
+
+ "github.com/aws/aws-sdk-go/aws"
+ "github.com/aws/aws-sdk-go/aws/credentials/stscreds"
+ "github.com/aws/aws-sdk-go/aws/session"
+ "github.com/aws/aws-sdk-go/service/ec2"
+
+ "github.com/weaveworks/eksctl/pkg/ami"
+ "github.com/weaveworks/eksctl/pkg/eks/api"
+
+ . "github.com/dave/jennifer/jen"
Dot imports are pretty anti-pattern and I don't see any obvious reasons
for it. The only general exception is in test code.
https://github.com/golang/go/wiki/CodeReviewComments#import-dot
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#273 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPWSx7VS-fL7b0h-FtGslTv2ydbBb60ks5umNpFgaJpZM4XtHsH>
.
|
@richardcase as you authored the original version, would you mind to give this a pass also? |
@errordeveloper - i'll have a look now |
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.
@errordeveloper - looks good, just one comment about a comment header. I would've thought jen would do this.
@@ -1,21 +1,18 @@ | |||
package ami |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
38eebcb
to
06bd20b
Compare
Due to GitHub outage, latest commits are not showing up yet... |
06bd20b
to
4ff1ca5
Compare
4ff1ca5
to
57c8178
Compare
Increase log verbosity for probe call
Description
I've used jenifer in the
--max-pods
code, and it seems to work nicely (although a little less intuitive then templates). One advantage is that it's all Go code, and it doesgofmt
for you also.Checklist
make build
)make test
)