-
Notifications
You must be signed in to change notification settings - Fork 6
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
minor: Add Leviathan Test Helpers #976
Conversation
sshKeyPath: join(homedir(), 'id'), | ||
sshKeyLabel: this.suite.options.id, | ||
sdk: new Balena(this.suite.options.balena.apiUrl, this.getLogger()), | ||
sdk: new Sdk(this.suite.options?.balena?.apiUrl, this.getLogger()), |
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 know that this is called sdk in the helpers package - bit isn't it a little bit more than just the SDK? We have some CLI uses in there, and some extra functions that are a composite of various sdk calls + CLI uses
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 is but I felt it was more confusing to import a component called Balena which is a file called sdk.js and initialize it as SDK.
Why confuse when we can straight away be called it SDK?
Coming over to what's inside of it, I think that should be broken down further in the helper's package to improve our consistency. The SDK helper should only have SDK methods. What do you think?
(This is the time and place we can semantically improve our nomenclature as well as structures) Happy to get more input on helpers package before merging this so we can get the best changes in there.
this all looks good to me - As far as I can see, there is no way for this to break anything other than leviathan e2e tests - which it won't as we can see those tests passing here. This is because you have not removed anything, in meta balena until the helpers are verified and that PR merges (balena-os/meta-balena#3091) they will continue to use the "baked-in" functions Maybe we can add a quic note to the PR description as to why we are making this change also |
Added the quick note and I completely agree with the nonbreaking changes with this PR. PR #826 was similar in this regard. |
Signed-off-by: Vipul Gupta (@vipulgupta2048) <[email protected]>
f95cb0f
to
f3f2752
Compare
@balena-ci I self-certify! |
To reduce Core's dependencies and in the end, simplify Leviathan architecture. We are taking the first steps to use the leviathan test helpers package in the e2e test suite. This will aid in removing Core's helpers #977 and helps provide a testbed to stabilize changes made in the test-helpers package.
Signed-off-by: Vipul Gupta (@vipulgupta2048) [email protected]