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

Added UpdateModel, publishModel,and unpublishModel functionality + tests #791

Merged
merged 4 commits into from
Feb 25, 2020

Conversation

ifielker
Copy link
Contributor

Added UpdateModel, publishModel,and unpublishModel functionality + tests

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just minor tidbits to update.

src/machine-learning/machine-learning-api-client.ts Outdated Show resolved Hide resolved
src/machine-learning/machine-learning-api-client.ts Outdated Show resolved Hide resolved
src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved
src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved
src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved
src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved
src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved
test/integration/machine-learning.spec.ts Show resolved Hide resolved
test/integration/machine-learning.spec.ts Show resolved Hide resolved
@hiranya911 hiranya911 assigned ifielker and unassigned hiranya911 Feb 21, 2020
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of nits.

}).then((x) => {
return x[0];
});
}).then((x) => x[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Replace x with something more descriptive. signResult or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.should.eventually.be.rejected.and.deep.equal(expected);
});

it('should resolve with the created resource on success', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/created/updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.stub(HttpClient.prototype, 'send')
.resolves(utils.responseFrom(OPERATION_SUCCESS_RESPONSE));
stubs.push(stub);
return apiClient.updateModel(MODEL_ID, NAME_ONLY_CONTENT, NAME_ONLY_MASK)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do an assertion on the URL and the body (Make sure the correct payload and mask is being passed to the backend)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@hiranya911 hiranya911 removed their assignment Feb 24, 2020
@ifielker ifielker merged commit 550ac02 into ml Feb 25, 2020
@ifielker ifielker deleted the ml-5 branch February 25, 2020 03:01
hiranya911 added a commit that referenced this pull request Apr 20, 2020
* Firebase ML Node.js SDK Structure (#778)

* Firebase ML Node.js SDK Structure
* added tests

* Added GetModel functionality and tests (#781)

* Added GetModel functionality and tests

* Added DeleteModel functionality and tests (#782)

* Added DeleteModel functionality and tests

* Added CreateModel functionality and tests (#788)

* Added CreateModel functionality and tests

* Added UpdateModel, publishModel,and unpublishModel functionality + tests (#791)

* Added UpdateModel, publishModel,and unpublishModel functionality plus tests

* Added ListModels functionality for Firebase ML (#795)

* Added ListModels functionality for Firebase ML

* Firebase ML changed endpoint (#813)

* Add ML APIs to docgen toc (#847)

* Docstring fixes and additions (#848)

* Docstring fixes and additions

* A few edits

* Ml merge (#851)

* Custom Action for sending Tweets (#784)

* Experimental custom Action for sending Tweets

* Added license headers

* Added README file

* Updated package descriptions

* Improve customClaims Typing (#768)

* chore: Experimental release flow based on Actions (#780)

* chore: Experimental release flow based on Actions

* Added tarball verification step; Simplified CI trigger

* Splitting staging and publish phases into separate jobs

* Fleshed out the full workflow

* Trigger RC build

* chore: Migrated to ESlint (#790)

* chore: Migrated to ESlint

* Added licesne header

* Enabling additional ESLint checks (#794)

* chore: Enabling more ESLint checks and fixing errors (#797)

* Fix compilation error in integration tests (#798)

Introduced by #790

* Build integration tests during CI (and release) (#800)

Note that this won't actually run them.

Additionally, the *unit* tests are also built, impying that we're
building them twice (once during this step, and possibly again when
running the unit tests.)

* Fix revokeRefreshTokens to round consistently with the other platforms. (#801)

This also makes it consistent with the comments a few lines above, as
well as the integration test.

* feat(auth): Multi-factor Auth support with SMS for Google Cloud Identity Platform (#804)

Defines multi-factor auth administrative APIs for Google Cloud Identity Platform.

* Defines new MFA types in toc.yaml. (#807)

* Removes special char from index.d.ts. (#808)

This is causing errors in the reference generation process.

* Defines MultiFactor{Create|Update}Settings interfaces. (#809)

* Defines MultiFactor{Create|Update}Settings interfaces.

* chore: Adding a .npmrc file to the root of the repo (#810)

* chore: Adding a .npmrc file to the root of the repo

* Removing the root-level .npmrc file

* [chore] Release 8.10.0 (#811)

* [chore] Release 8.10.0 (take 2) (#812)

* Bump acorn from 6.1.1 to 6.4.1 (#815)

Bumps [acorn](https://github.com/acornjs/acorn) from 6.1.1 to 6.4.1.
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@6.1.1...6.4.1)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fixing Android notification options descriptions (#820)

* Fixing doc bug that conflated sound and tag options for Android notifications.

* Removing duplicate documentation for tag.

* Adding tag details in the right place this time, hopefully.

* chore: Splitting the index.d.ts file into smaller files (#751)

* Splitting the index.d.ts file into smaller files

* Database return type fixed

* chore: Cleaning up package verification scripts (#822)

* chore: Cleaning up package verification scripts

* Added package metadata to test package.json file

* fix(auth): Fixing UserImportRecord typings declaration (#835)

* fix(auth): Fixing UserImportRecord typings declaration

* Fixing more integration test compilation errors

* Trigger CI

* Removed redundant line

* Bump minimist from 1.2.0 to 1.2.3 (#839)

Bumps [minimist](https://github.com/substack/minimist) from 1.2.0 to 1.2.3.
- [Release notes](https://github.com/substack/minimist/releases)
- [Commits](https://github.com/substack/minimist/compare/1.2.0...1.2.3)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* remerge conflict

Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com>
Co-authored-by: William Sedlacek <wsedlacekc@gmail.com>
Co-authored-by: rsgowman <rgowman@google.com>
Co-authored-by: bojeil-google <bojeil-google@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: egilmorez <egilmore@google.com>

* Added Firebase ML API requirements to Contributing doc (#853)

Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com>
Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com>
Co-authored-by: William Sedlacek <wsedlacekc@gmail.com>
Co-authored-by: rsgowman <rgowman@google.com>
Co-authored-by: bojeil-google <bojeil-google@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: egilmorez <egilmore@google.com>
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