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

docs(samples): Talent API Sample Revision & Test #1546

Merged
merged 20 commits into from Jan 30, 2019
Merged

docs(samples): Talent API Sample Revision & Test #1546

merged 20 commits into from Jan 30, 2019

Conversation

josecervino
Copy link

@josecervino josecervino 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?

@josecervino
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 :)

@josecervino
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.


it('should create a company', async () => {
const scope = nock(Utils.baseUrl)
.post(`/v3/jobs/project/project-id/companies`)
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

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

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.

None yet

4 participants