-
Notifications
You must be signed in to change notification settings - Fork 2k
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
chore(vision): migrate code from googleapis/nodejs-vision #2858
Conversation
* Switch back to mocha from ava. * Make lint happy. * Uncache vision. * Make canvas dependency optional. * Change node version. * Remove const
* Refactor tests. * Tweak build. * Tweak build. * More tests. * Tweak build. * Tweak build. * Fix build. * Fix build. * Speed up build. * Fix build. * Remove extra dep. * Investigate why 0.12 fails. * Scripts. * More tests. * Upgrades * Upgrades * Update readme
…e new Cloud Storage samples (#164) * Brought Pub/Sub samples code coverage up to 98.66%
* Switch to individual API packages. * Address comments * Address comments
* New quickstarts. * Address comments.
* Update storage samples. * Update dependencies.
* Switch from Mocha to Ava * Concurrency: 5
* Make unify script recursive + clean up dependency conflicts * Restore travis.yml * Delete outdated text detection sample that duplicates detect.js * Fix failing KMS + vision tests by updating dependencies * Fix video tests using a bad cwd * Upgrade monitoring dependency + skip flaky monitoring tests * Fix DLP tests having wrong cwd * Fix failing vision test * Fix datastore tests * Update broken dependency * Update possibly broken compute engine dependency * Fix typos * Disable Node 4 testing * Revert deletion of outdated sample - @gguuss says we still use this. This reverts commit b7259c8. * Update dependency
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Thanks for preparing this draft, Eric. |
Yep, that's why it was in a draft PR -- I wasn't done working on it 🙂 |
Here is the summary of changes. You are about to add 61 region tags.
This comment is generated by snippet-bot.
|
.github/workflows/workflows.json
Outdated
"datalabeling", | ||
"datastore/functions", | ||
"datacatalog/quickstart", | ||
"datastore/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.
Nitpick: datastore/functions
appears twice in this list.
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 catch!
"service-directory/snippets", | ||
"scheduler", | ||
"speech", | ||
"talent", | ||
"translate", | ||
"video-intelligence", | ||
"contact-center-insights", | ||
"vision", | ||
"vision/productSearch", |
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.
Question 1: Should vision/productSearch
actually be vision/samples/productSearch
?
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.
You're correct. Done.
"service-directory/snippets", | ||
"scheduler", | ||
"speech", | ||
"talent", | ||
"translate", | ||
"video-intelligence", | ||
"contact-center-insights", | ||
"vision", | ||
"vision/productSearch", | ||
"workflows" | ||
] |
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.
Praise: Thank you for sorting this in alphabetical order, Eric.
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.
👍🏻
paths: | ||
- 'vision/**' | ||
pull_request: | ||
paths: |
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.
Please add vision/productSearch/**
to paths-ignore
so changes to vision-productSearch don't trigger tests of vision.
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.
Where would I find the definition for this YAML schema? I've made the change to the YAML file but I don't think it's correct ....
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.
.github/workflows/workflows.json
Outdated
@@ -48,20 +51,17 @@ | |||
"healthcare/fhir", | |||
"healthcare/hl7v2", | |||
"kms", | |||
"game-servers/snippets", |
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.
Praise: Thank you for alphabetizing this config file.
Suggestion: Changes like this are our of scope of the PR, consider putting reorganization work in a separate PR, especially in this context where individual branch commits will be retained as part of the repository history.
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.
Okay, fair enough. I've reverted the alphabetization.
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
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.
Because there is no existing vision sample for which we want to avoid collision, the target path should be /vision
, not /vision/samples
.
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.
Done.
gcloud app
instead ofgcloud preview app
#148)transaction.get
guarantees the same order as the keys? #234)