-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add find_ami_id logic instead of pass argument --instance-id
#167
Add find_ami_id logic instead of pass argument --instance-id
#167
Conversation
d36bdd4
to
018caa8
Compare
Above address Sean's comment.
|
eks_version: &str, | ||
) -> ProviderResult<String> { | ||
let parameter_name = format!( | ||
"/aws/service/bottlerocket/aws-k8s-{}/{}/{}/image_id", |
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.
Maybe define this as a pattern variable?
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.
Can you give an example about it? thanks
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.
Something similar to
const PARAMETER_PATERN = "/aws/service/bottlerocket/aws-k8s-{}/{}/{}/image_id"
let parameter_name = format!(PARAMETER_PATERN, ...)
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.
I don't think we can use format!(PARAMETER_PATERN, ...)
since format! needs syntax like format!({}, ...)`. We can change it to
const PARAMETER_PATERN = "/aws/service/bottlerocket/aws-k8s-"
let parameter_name = format!({}{}/{}/{}/image_id, PARAMETER_PATERN, ...)
But I don't think this is readable, so I would keep current format. Thanks. 👍
0a3e69b
to
8536153
Compare
currently integration test needs human manually find bottlerocket ami id and pass it to integration test via `--instance-id`, which isn't efficient and convenient; therefore, we add a new logic about finding ami id via aws-sdk-ssm according to arch, bottlerocket-version, and eks-version provided by the users.
8536153
to
6292225
Compare
Issue number:
#162
Description of changes:
Testing done:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.