Skip to content
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

Update to the v7 model #1420

Merged
merged 28 commits into from
Aug 30, 2024
Merged

Update to the v7 model #1420

merged 28 commits into from
Aug 30, 2024

Conversation

thgreasi
Copy link
Member

@thgreasi thgreasi commented Jun 20, 2024

@thgreasi thgreasi changed the title V7 Update to the v7 model Jun 20, 2024
@thgreasi thgreasi requested a review from JSReds June 20, 2024 16:08
@@ -81,13 +76,6 @@ export interface OsVersion
basedOnVersion?: string;
osType: string;
line?: OsLines;
/** @deprecated */
isRecommended?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reimplement it back as we discussed last time. This will keep the logic centralised and avoid having to replicate the "isRecommended" method in parts that uses it.

@JSReds
Copy link
Contributor

JSReds commented Jul 23, 2024

testing it I've noticed that now we cannot make any requests without passing at least a $select, is this wanted ?
if so we should probably make options to be mandatory as methods like getAllByApplication are not working anymore:

await sdk.models.device.getAllByApplication(<FLEET_ID>)

request.js:192 Uncaught BalenaRequestError: Request error: Must include a $select for top level 'device'
    at Object.eval (request.js:191:27)
    at Generator.next (<anonymous>)
    at fulfilled (tslib.es6.mjs:153:56)
        

@thgreasi
Copy link
Member Author

thgreasi commented Jul 28, 2024

@JSReds are you using /v7 from the API PR or are you targeting /resin ? $selects should only be necessary on the /resin model.

When I tested the sdk against v7 on my bM last weekend, the tests cases passed as they are (w/o adding the missing $selects).
Moreover there are some test cases in the API on the v7 branch that do pass w/o a $select.
See: https://github.com/balena-io/balena-api/blob/7b5af1a2e1a22b968fc381b06e614484bc410d9f/test/01_user.ts#L1999

@JSReds
Copy link
Contributor

JSReds commented Jul 31, 2024

got it @thgreasi, I'm effectively using /resin, this is probably because of it :) thanks for the clarification 🙏

@thgreasi
Copy link
Member Author

I've just added test cases to make sure v7 works w/o $selects as well in the API PR :)
See: https://github.com/balena-io/balena-api/pull/5120/files#diff-50786f06ef687c91c0d34ed511be85399f074d7b4cd4387900c26bc15f35afa7R86

Update pinejs-client-core from 6.14.0 to 6.15.0

Change-type: patch
…h should_be_managed_by__release

Change-type: major
…ed property when selected

Change-type: major
…ted, reduced-functionality & operational

Change-type: major
@thgreasi thgreasi marked this pull request as ready for review August 30, 2024 06:54
@thgreasi thgreasi merged commit aac3ae1 into master Aug 30, 2024
53 checks passed
@thgreasi thgreasi deleted the v7 branch August 30, 2024 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants