-
Notifications
You must be signed in to change notification settings - Fork 15
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
Integration test for disable default cni flag #113
Integration test for disable default cni flag #113
Conversation
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 start but there is work left to be done
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.
Going in the right direction, added a couple of comments.
In general, have you validated that it is fine to remove those environment variables and hardcode the values in the templates? e.g. are we only using this once and nowhere else?
@@ -4,40 +4,10 @@ | |||
|
|||
The integration/e2e tests have the following prerequisites: | |||
|
|||
* an environment variable `CLUSTER_MANIFEST_FILE` pointing to the cluster manifest. Cluster manifests can be produced with the help of the templates found under `templates`. For example: |
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 those prerequisites not required anymore?
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.
yes those are not required they are hardcoded in cluster manifest dir
yest they are used only in tests |
removed unnecessary functions
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.
Couple doc tweaks for your consideration, but otherwise LGTM. Thanks @maci3jka!
* make sure to have ssh key in aws `capi`in `us-east-1 region` if you do not have key refer | ||
to CAPI on [AWS prerequisites documentation](https://cluster-api-aws.sigs.k8s.io/topics/using-clusterawsadm-to-fulfill-prerequisites#ssh-key-pair) |
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.
* make sure to have ssh key in aws `capi`in `us-east-1 region` if you do not have key refer | |
to CAPI on [AWS prerequisites documentation](https://cluster-api-aws.sigs.k8s.io/topics/using-clusterawsadm-to-fulfill-prerequisites#ssh-key-pair) | |
* ensure an AWS ssh key pair named `capi` exists according to the [CAPI Provider AWS documentation](https://cluster-api-aws.sigs.k8s.io/topics/using-clusterawsadm-to-fulfill-prerequisites#ssh-key-pair) |
Co-authored-by: Kevin W Monroe <[email protected]>
7470d77
to
f313dcc
Compare
Added tests with disableDefaultCNI set to true for change #110 and improved loging.
Readme fixes.
KU-1226