-
Notifications
You must be signed in to change notification settings - Fork 120
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 Integration tests #618
Conversation
@rkjakeer Thank you for your contribution. |
Thank you @rkjakeer for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
Hi @rkjakeer , Thanks for this PR. Might need a couple of days to have a look. |
@prashanth26 I reached out successfully to the cla-assistant to recheck this pull request. |
Also, I think your commits are coming from different user names. Please try to amend the commits to make them originate from a single ID. Also do sign the CLA as requested by the bot. |
@rkjakeer You need rebase this pull request with latest master branch. Please check. |
No change required for IT - #623. |
@rkjakeer You need rebase this pull request with latest master branch. Please check. |
Hi @rkjakeer, Thank you for this amazing PR! I absolutely loved the simplicity of usage. Please find two review comments from my side from my first execution of this suite. And maybe, you can combine the related, relevant code changes into a fewer commits Summary:
|
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.
Hi @rkjakeer @toniajuliejackson ,
Apologies for the delay in reviewing. The tests look really good. We really appreciate the changes. I just have a few minor comments. Looks good to me otherwise.
Also lastly it would be great to split the PR into two commits before the final merge. One with your actual changes and the other with vendoring changes. |
Hi @prashanth26, No problem. I have made the changes as suggested. |
24d6f52
to
41b35e8
Compare
@rkjakeer You need rebase this pull request with latest master branch. Please check. |
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 the changes.
/lgtm overall, @AxiomSamarth and @himanshu-kun seem to have tested this. We might need minor refactoring in the future to accommodate this into your CI pipelines.
* Add integrationtest directory * Use rti interface for integration testing * update makefile Co-Authored-By: Tonia Jackson <[email protected]>
@prashanth26 I tested this suite against AWS and Azure clusters. Test Suite succeeded for both. This could be merged now. |
@AxiomSamarth - Thank you for this. We can merge once @rkjakeer confirms. |
What this PR does / why we need it:
This PR adds a set of common Integration test cases as a framework for all machine-controller-manager-provider-{provider-name} repositories.
Which issue(s) this PR fixes:
Fixes #216
Special notes for your reviewer:
Release note: