Skip to content

Conversation

slz250
Copy link
Contributor

@slz250 slz250 commented May 21, 2020

In the create call demo -- allow uncommenting of POST check code. This is a new feature and will help drive adoption.

In the create call demo -- allow uncommenting of POST check code. This is a new feature and will help drive adoption.
@slz250 slz250 requested a review from a team as a code owner May 21, 2020 18:44
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2020
@gguuss gguuss added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 21, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 21, 2020
Copy link
Contributor

@gguuss gguuss left a comment

Choose a reason for hiding this comment

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

LGTM as lint is passing and this is just a comment change.

Do you want to just expose this as a comment or would it make sense to add an additional example and also add a test for the POST check?

It may be worth getting someone closer to monitoring on the DPE side for a second approval.

@tmatsuo tmatsuo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 1, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 1, 2020
@tmatsuo
Copy link
Contributor

tmatsuo commented Jun 1, 2020

As @gguuss mentioned, it might be better to have another sample with a test.
If it's in the comment, there's no way to know if it's working.

Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

@slz250 +1 to @tmatsuo's comment. Could you create a new sample that creates an uptime check config with a post check?

We prefer not to have code commented out because it's not possible to test it. If the library changes for some reason, this sample could break.

@leahecole
Copy link
Collaborator

#4082 was just merged

@leahecole
Copy link
Collaborator

So I definitely did not intentionally unassign @gguuss - it did that when I commented, and now I can't re-add him. 😬

@slz250 slz250 closed this Jul 7, 2020
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.

8 participants