-
Notifications
You must be signed in to change notification settings - Fork 19
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
added sample test config files #740
added sample test config files #740
Conversation
3ed2fdb
to
865e5a0
Compare
5fd97a7
to
d0c37dc
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.
Great work. Can we make cluster_name
configurable for all crds and have the crds names be related to the cluster name?
800b54f
to
3d4fa18
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.
Just a few more nits. Great work so far.
samples/RUNBOOK.md
Outdated
export UPGRADE_VERSION="v1.11.1" | ||
export STARTING_VERSION="v1.11.0" |
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'm wondering if these should have similar logic to AGENT_IAMGE_VERSION
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.
It won't be as simple as calling something like cli --version
. Maybe a curl
call to public.ecr.aws/bottlerocket/<agent-image>
and figure out the latest versions that way?
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.
Let's save that for a separate pr.
a12ddd3
to
541b478
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.
LGTM 🎉
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.
Are these mostly for documentation purposes?
My concern is that the chance of these being incorrect in the future is about 100%.
Can we generate them?
The samples will be used to validate the agents before a new version is released. So they will always be correct with the latest released version. |
Is there any way to apply them to a kind cluster during CI integ tests? We know that the agents in question will not exist and the controller will report errors, but being able to apply these to an integ cluster would ensure they are not out-of-date. |
That would just validate the crd, it wouldn't validate the individual agents configurations. A followup to this will include tests, deserializing each piece of the manifest and verifying that the configuration matches the agents configuration. (I think that's what you are asking for) We could also add automatic generation, but I think this is a good first step. |
Sorry, yes that's even better. Unit tests that deserialize these is what I'm looking for. Any reason not to include those tests now? I'd hate to see that fall by the wayside. |
We'd like to use these for the next release. I'm not sure how much effort the unit tests will require. |
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.
The bash snippets with all the sed
s is quite unsightly and a bit to much for something I would be comfortable copying and running directly.
I wonder if there's a better way to go about with the templating short of just going full helm charts.
I think I would prefer using heredoc
s more so it's cleaner. For example,
First, change the variables like <CLUSTER_NAME>
to ${CLUSTER_NAME}
in the template yaml file. Then...
### Migration Testing on `aws-ecs` Variants
CLUSTER_NAME="x86-64-aws-ecs-1"
OUTPUT_FILE="${CLUSTER_NAME}-migration.yaml"
VARIANT="aws-ecs-1"
ARCHITECTURE="x86_64"
#....
eval "cat > ${OUTPUT_FILE} <<EOF
$(<eks/ecs-migration-test.yaml)
EOF
" 2> /dev/null
Should give you what you want without all the sed
s. Please try it out.
541b478
to
2a5e6d0
Compare
2a5e6d0
to
120970a
Compare
^ Add a unit test that will ensure one of the sample config files stays up-to-date. https://github.com/bottlerocket-os/bottlerocket-test-system/compare/2a5e6d06a49ee740d106a67c736c5f31afcf6089..120970a8e60ff6cacafb58fe5f8c24c368042979 |
We were looking at this together while writing a unit test. The problem is that we have used this syntax |
120970a
to
15b6604
Compare
^ Added the unit tests for the remaining sample files. |
15b6604
to
9675e37
Compare
^ Replaced the many |
9675e37
to
7cc4897
Compare
7cc4897
to
c8d25b6
Compare
^ Changed |
Issue number:
N/A
Description of changes:
Added sample YAML files to be used with
cli
, as well as a runbook that shows how to populate and use the files.Testing done:
ecs-migration-test.yaml
:ecs-test.yaml
:sonobuoy-migration-test.yaml
:sonobuoy-test.yaml
with SONOBUOY-MODE "quick":vmware-migration-test.yaml
:vmware-sonobuoy-test.yaml
: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.