-
Notifications
You must be signed in to change notification settings - Fork 69
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
EKS support for prombench #384
EKS support for prombench #384
Conversation
e70eca2
to
f13f7ea
Compare
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.
nice work! :) some changes and questions.
57db3bc
to
8aa54a9
Compare
@geekodour Can you please review it again? |
@geekodour Docmentation added. |
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.
Great stuff, nice improvements :) Thanks! ✨
Some questions and change suggestions.
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.
Good progress! thanks for the work 😃
Added some suggestions, questions and ideas. :)
212bf71
to
118eb65
Compare
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.
We're almost nearing finishing this PR, I think a couple of more iterations :) Nice work 👍
Some suggestion, questions and changes.
pkg/provider/eks/eks.go
Outdated
// NewEKSClient sets the EKS client used when performing the GKE requests. | ||
func (c *EKS) NewEKSClient(*kingpin.ParseContext) error { | ||
if c.AuthFilename != "" { | ||
} else if c.AuthFilename = os.Getenv("AWS_APPLICATION_CREDENTIALS"); c.AuthFilename == "" { |
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.
Why do we need to check for AWS_APPLICATION_CREDENTIALS
?
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.
Actually this seems like a good idea: https://docs.github.com/en/actions/configuring-and-managing-workflows/creating-and-storing-encrypted-secrets#using-encrypted-secrets-in-a-workflow
But we'll still be passing the base64 version of the toml
credentials file in Github Actions, so we still need to see if the auth data is base64 encoded and create a file to pass to NewSharedCredentials
.
test-infra/pkg/provider/gke/gke.go
Lines 92 to 103 in e7f8c93
// Check if auth data is base64 encoded and decode it. | |
encoded, err := regexp.MatchString("^([A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{3}=|[A-Za-z0-9+/]{2}==)?$", c.Auth) | |
if err != nil { | |
return err | |
} | |
if encoded { | |
auth, err := base64.StdEncoding.DecodeString(c.Auth) | |
if err != nil { | |
return errors.Wrap(err, "could not decode auth data") | |
} | |
c.Auth = string(auth) | |
} |
c1120bc
to
5c1e310
Compare
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.
Thanks for adding the changes! Added some comments to make this PR smaller by extracting out the GKE specific changes to a separate PR :)
@geekodour Added info command for eks Waiting for #425 to be merged 😝 |
7a302e8
to
cbee5c1
Compare
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 like this is shaping up nicely, some minor documentation nits :)
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.
Nice, almost LGTM :) Few questions and minor changes. 🎇 Please update the PR description with all the new changes and remove the changes that are no longer part of the PR. Also mention about automated subnet ids will add support of funcbench etc.
@krasi-georgiev It'll be awesome if you can take a look at this one :)
8adf862
to
2615217
Compare
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
Signed-off-by: Drumil Patel <[email protected]>
0783336
to
44870e4
Compare
Signed-off-by: Drumil Patel <[email protected]>
This PR incorporates the following changes:-