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

Moved Logs sample to Github #166

Merged
merged 4 commits into from
Apr 20, 2016
Merged

Conversation

Annie29
Copy link
Contributor

@Annie29 Annie29 commented Apr 20, 2016

Example from Logs (https://cloud.google.com/appengine/docs/java/logs/) moved to Github. No tests yet.

import java.io.PrintWriter;
import java.util.Calendar;

//import javax.servlet.http.*; // Fix these to remove * import
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should remove this line.

@lesv
Copy link
Contributor

lesv commented Apr 20, 2016

Good to either assign to me, (or someone else if I'm not available). Or at least mention us using a @name.

Please remove the two commented import statements.

LGTM - once done. Feel free to merge.

For comments, stuff about the API's is good, or anything tricky, but assume they know the language and how to program. (however, I'd much rather have too many, than too few.

@lesv
Copy link
Contributor

lesv commented Apr 20, 2016

@tswast Why didn't travis or the cla bots run here?

@Annie29 Please wait for Travis to run before merging.

@googlebot
Copy link

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).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@Annie29
Copy link
Contributor Author

Annie29 commented Apr 20, 2016

@lesv Sorry to miss notifying you by name. I'll work on that. The import lines are fixed, but Travis still fails. I had updated the main pom.xml file. I still don't get what the iv error really means. Maybe @swast can drop by and show me how to fix this if he has some time.

@@ -0,0 +1,39 @@
# Users Authentication sample for Google App Engine

This sample demonstrates how to use the [Logs API][appid] on [Google App
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend something like [Logs API][log-docs] instead of [appid].

@tswast
Copy link
Contributor

tswast commented Apr 20, 2016

A few style nits, but LGTM overall.

In the future, please create a new branch on this repo and make the pull request from there. That way Travis can run happily. Also, the CLA bot is not happy. You should make your GitHub profile use your @google.com email address per the Open Source policy.

@lesv I'm not sure sure why Travis didn't run right away, but it appears that they and the GoogleBot caught up.

@lesv
Copy link
Contributor

lesv commented Apr 20, 2016

@tswast Travis seems very broken at the moment.

@tswast
Copy link
Contributor

tswast commented Apr 20, 2016

Travis is unhappy because it's from a fork instead of a branch. I have issue #117 open to fix that. Once the last fixes are in, I can run the tests manually before merging.

Replaced Calendar class with org.joda.time.DateTime.
@laurieaw
Copy link

@lesv, @tswast Let's try this again. Since this isn't a branch, I'll probably still have Travis trouble.

@tswast
Copy link
Contributor

tswast commented Apr 20, 2016

LGTM. Thank you! I'll merge after the tests pass on my local machine.

@tswast
Copy link
Contributor

tswast commented Apr 20, 2016

Tests are failing due to testGetAllUsers(com.google.appengine.sparkdemo.UserControllerTest) in the sparkdemo tests, but that's unrelated to this change. It does get through all the tests except that one. I'll merge and hope it passes on Travis.

@tswast tswast merged commit ced4fa6 into GoogleCloudPlatform:master Apr 20, 2016
bourgeoisor pushed a commit that referenced this pull request Nov 11, 2022
…1.0 (#166)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [com.google.cloud:libraries-bom](https://github.com/GoogleCloudPlatform/cloud-opensource-java) | minor | `13.0.0` -> `13.1.0` |

---

### Renovate configuration

:date: **Schedule**: At any time (no schedule defined).

:vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

:recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-notification).
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.

5 participants