Skip to content

Conversation

@joecervino
Copy link

@joecervino joecervino commented Jan 14, 2019

Fixes #1296

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 14, 2019
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

Thanks! 🎉

@JustinBeckwith JustinBeckwith changed the title Samples: Talent API Sample Revision & Test docs(samples): Talent API Sample Revision & Test Jan 14, 2019
@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 14, 2019
@JustinBeckwith JustinBeckwith added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jan 14, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 14, 2019
@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 21, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 21, 2019
@JustinBeckwith
Copy link
Contributor

👋 @JCIV-SE it looks like the sample tests are failing. Can you take a look please?

@joecervino
Copy link
Author

@JustinBeckwith Carrying over the outdated duplicate PR's discussion:

  • Relevant code in the sample
  const projectId = await google.auth.getProjectId();
  const res = await jobService.projects.companies.create({
    parent: `project/${projectId}`,
    requestBody: {
  • Relevant code in the test
    const scope = nock(Utils.baseUrl)
                      .post(`/v3/jobs/project/project-id/companies`)
                      .reply(200, {});
    const data = await samples.jobs.runSample();

I'm mirroring a lot of the code from other samples. The biggest difference is the url passed to .post() in the test where I hard-code the project-id to the one used for testing in the repo. Using a template literal + project-id variable doesn't work well there. The company creation method constructs the url to access the company directly using the parent property and the request body properties - do you think it has to do with that? I'm new to Nock so the nuances aren't readily apparent yet :)

@JustinBeckwith
Copy link
Contributor

🤷‍♂️ If you can push to a branch on upstream, I can clone it and take a look :)

@joecervino
Copy link
Author

@JustinBeckwith Don't have access to the upstream repo like that :l you can clone the forked branch here: https://github.com/JCIV-SE/google-api-nodejs-client/tree/samples/talentapi

@JustinBeckwith
Copy link
Contributor

@JCIV-SE I just invited you to the googleapis org. Let me know when you accept, and I can get you added to the correct groups.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I think this is the problem. For this url, can you use ${process.env.GCLOUD_PROJECT}?

Copy link
Author

@joecervino joecervino Jan 23, 2019

Choose a reason for hiding this comment

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

Good idea! I tried it several different ways though and I'm still getting:

FetchError: request to https://jobs.googleapis.com/v3/project/project-id/companies failed, 
reason: Nock: Disallowed net connect for "jobs.googleapis.com:443/v3/project/project-id/companies"

🎊

I accepted the invitation (thanks!), I'll try pushing a branch upstream once auth allows it

Copy link
Contributor

Choose a reason for hiding this comment

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

huh weird. Share your updated code?

Copy link
Author

@joecervino joecervino Jan 23, 2019

Choose a reason for hiding this comment

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

Sure! Here's version 1:

  it('should create a company', async () => {
    const scope = nock(Utils.baseUrl)
                      .post(`/v3/jobs/project/${process.env.GCLOUD_PROJECT}/companies`)

version 2:

  it('should create a company', async () => {
    const scope = nock(Utils.baseUrl)
                      .post(`${process.env.GCLOUD_PROJECT}`)

I tried a few other ways but the above two are the ones that make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, v1 looks right to me

Copy link
Author

Choose a reason for hiding this comment

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

Some logging revealed that GCLOUD_PROJECT is undefined until we go into runSample's scope where we call on the auth service to generate a project Id - copying the same code ( const projectId = await google.auth.getProjectId();) into the test's scope gives us undefined too so.. any chance we can erase this test and push the sample up anyway? A bit anarchistic but I noticed there are a few other APIs without them

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the test, just put a .skip on it :)

Copy link
Author

Choose a reason for hiding this comment

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

Cool! Done!

@JustinBeckwith JustinBeckwith merged commit d1bf1b2 into googleapis:master Jan 30, 2019
gcf-owl-bot bot added a commit that referenced this pull request Aug 23, 2022
because the tools are already installed in the docker image as of googleapis/testing-infra-docker#227
Source-Link: googleapis/synthtool@ab7384e
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:bb493bf01d28519e82ab61c490c20122c85a7119c03a978ad0c34b4239fbad15
gcf-owl-bot bot added a commit that referenced this pull request Aug 24, 2022
* fix: add hashes to requirements.txt

and update Docker images so they require hashes.

* fix: add hashes to docker/owlbot/java/src

* Squashed commit of the following:

commit ab7384ea1c30df8ec2e175566ef2508e6c3a2acb
Author: Jeffrey Rennie <rennie@google.com>
Date:   Tue Aug 23 11:38:48 2022 -0700

    fix: remove pip install statements (#1546)

    because the tools are already installed in the docker image as of googleapis/testing-infra-docker#227

commit 302667c9ab7210da42cc337e8f39fe1ea99049ef
Author: WhiteSource Renovate <bot@renovateapp.com>
Date:   Tue Aug 23 19:50:28 2022 +0200

    chore(deps): update dependency setuptools to v65.2.0 (#1541)

    Co-authored-by: Anthonios Partheniou <partheniou@google.com>

commit 6e9054fd91d1b500cae58ff72ee9aeb626077756
Author: WhiteSource Renovate <bot@renovateapp.com>
Date:   Tue Aug 23 19:42:51 2022 +0200

    chore(deps): update dependency nbconvert to v7 (#1543)

    Co-authored-by: Anthonios Partheniou <partheniou@google.com>

commit d229a1258999f599a90a9b674a1c5541e00db588
Author: Alexander Fenster <fenster@google.com>
Date:   Mon Aug 22 15:04:53 2022 -0700

    fix: update google-gax and remove obsolete deps (#1545)

commit 13ce62621e70059b2f5e3a7bade735f91c53339c
Author: Jeffrey Rennie <rennie@google.com>
Date:   Mon Aug 22 11:08:21 2022 -0700

    chore: remove release config and script (#1540)

    We don't release to pypi anymore.

* chore: rollback java changes

to move forward with other languages until Java's docker image is fixed
Source-Link: googleapis/synthtool@4826337
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:7fefeb9e517db2dd8c8202d9239ff6788d6852bc92dd3aac57a46059679ac9de
SurferJeffAtGoogle pushed a commit that referenced this pull request Aug 30, 2022
* fix: add hashes to requirements.txt

and update Docker images so they require hashes.

* fix: add hashes to docker/owlbot/java/src

* Squashed commit of the following:

commit ab7384ea1c30df8ec2e175566ef2508e6c3a2acb
Author: Jeffrey Rennie <rennie@google.com>
Date:   Tue Aug 23 11:38:48 2022 -0700

    fix: remove pip install statements (#1546)

    because the tools are already installed in the docker image as of googleapis/testing-infra-docker#227

commit 302667c9ab7210da42cc337e8f39fe1ea99049ef
Author: WhiteSource Renovate <bot@renovateapp.com>
Date:   Tue Aug 23 19:50:28 2022 +0200

    chore(deps): update dependency setuptools to v65.2.0 (#1541)

    Co-authored-by: Anthonios Partheniou <partheniou@google.com>

commit 6e9054fd91d1b500cae58ff72ee9aeb626077756
Author: WhiteSource Renovate <bot@renovateapp.com>
Date:   Tue Aug 23 19:42:51 2022 +0200

    chore(deps): update dependency nbconvert to v7 (#1543)

    Co-authored-by: Anthonios Partheniou <partheniou@google.com>

commit d229a1258999f599a90a9b674a1c5541e00db588
Author: Alexander Fenster <fenster@google.com>
Date:   Mon Aug 22 15:04:53 2022 -0700

    fix: update google-gax and remove obsolete deps (#1545)

commit 13ce62621e70059b2f5e3a7bade735f91c53339c
Author: Jeffrey Rennie <rennie@google.com>
Date:   Mon Aug 22 11:08:21 2022 -0700

    chore: remove release config and script (#1540)

    We don't release to pypi anymore.

* chore: rollback java changes

to move forward with other languages until Java's docker image is fixed
Source-Link: googleapis/synthtool@4826337
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:7fefeb9e517db2dd8c8202d9239ff6788d6852bc92dd3aac57a46059679ac9de

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants