Skip to content

GCF: Add Slack sample + clean up imports #2394

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Mar 21, 2020
Merged

GCF: Add Slack sample + clean up imports #2394

merged 22 commits into from
Mar 21, 2020

Conversation

ace-n
Copy link
Contributor

@ace-n ace-n commented Mar 13, 2020 •
edited
Loading

Do not merge until this has actually been tested with Slack.

(@lesv I added you since Kurtis is OOO - LMK if I should ping someone else!)

@ace-n ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 13, 2020
@ace-n ace-n requested review from lesv, averikitsch and a team March 13, 2020 06:51
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 13, 2020
lesv
lesv suggested changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

PTAL
I've sent you a list of books. The main one is Effective Java 3rd ed.
If your POM inherits from our shared-configuration:

<dependency>
  <groupId>com.google.cloud.samples</groupId>
  <artifactId>shared-configuration</artifactId>
  <version>1.0.12</version>
  <type>pom</type>
</dependency>

You should be able to domvn checkstyle:check.

ace-n
ace-n commented Mar 14, 2020
View reviewed changes
Copy link
Contributor Author

@ace-n ace-n left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I had to disable surefires automatic testing via skipTests in pom.xml to deploy the code. (The tests rely on environment variables, which aren't provisioned until the tests finish on GCF - a catch-22.)

That seems to disable local tests too (e.g. when mvn clean verify is called), which is not what I wanted. I assume we can disable tests only on GCF deploys, but I don't know what the best way to do this is.

(Arguably, the GCF deployment process shouldn't be calling test goals.)

@ace-n
Copy link
Contributor Author

ace-n commented Mar 16, 2020

(Related question: should we document how to include files alongside your function like we do for other languages?)

@lesv
Copy link
Contributor

lesv commented Mar 16, 2020

re-disabling surefire, we should enable but set appropriate defaults so users can run.

lesv
lesv reviewed Mar 16, 2020
View reviewed changes
lesv
lesv reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

Once you figure out surefire, LGTM

@ace-n
Copy link
Contributor Author

ace-n commented Mar 17, 2020 •
edited
Loading

I've tried to trigger profiles (for "only test locally" purposes) based on three things:

  1. Maven archetypes - the Java functions framework deploys the function using mvn functions:deploy as shown here. However, this StackOverflow post says goal-dependent triggering is not possible.
  2. Environment variables - these are set after the function build (and test run) is complete, so you can't use them as a proxy for "am I on dev or prod?" (Alternatively, we could check for env variables that are defined on a user's system regardless of OS - e.g. $HOME or $SHELL)
  3. File presence - if .gcloudignore is present, run the tests. This does work, but it seems kinda hacky - especially vs. option 1.

@lesv thoughts?

May 2020 update: we decided to go with env-var-based profiles, specifically skipping tests when env.NEW_BUILD is defined (which is the case on Cloud Functions, but not Cloud Build).

lesv
lesv reviewed Mar 18, 2020
View reviewed changes
@ace-n ace-n changed the title [WIP] GCF: Add Slack sample + clean up imports GCF: Add Slack sample + clean up imports Mar 20, 2020
@ace-n ace-n removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 20, 2020
@ace-n ace-n merged commit 1e5e99a into master Mar 21, 2020
@ace-n ace-n deleted the gcf-slack branch March 21, 2020 00:45
olavloite pushed a commit to olavloite/java-docs-samples that referenced this pull request Mar 22, 2020
@olavloite
GCF: Add Slack sample + clean up imports (GoogleCloudPlatform#2394)
e3ee897 
* Add Slack sample + clean up imports

* Address comments

* Remove excess gcloudignore + actually disable tests

* Simplify tests + run them on Kokoro. ALSO bugfix unused shellchecks.

* Remove extra file

* HACK: resolve surefire issue via file presence

* HACK take 2: use a different filepath

* HACK take 3: use env var not used by local Cloud Build

* Remove gitignore now that config.json isnt used

* DBG: print defined env vars

* DBG take 2

* DBG take 3

* DBG take 4

* DBG take 5

* DBG take 6

* DBG take 7

* Fix tests...?

* Revert dbg commits + fix tests
@ace-n ace-n mentioned this pull request Mar 25, 2020
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request Mar 25, 2020
GCF Slack: add javadoc + region tags (#2486)
daa2ce4 
Forgot these in #2394 - cc @averikitsch @grant as FYI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@averikitsch averikitsch Awaiting requested review from averikitsch

1 more reviewer

@lesv lesv lesv approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ace-n @lesv @gguuss @googlebot