-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
GCF: Add Slack sample + clean up imports #2394
Conversation
There was a problem hiding this 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
.
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to disable surefire
s 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.)
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java
Outdated
Show resolved
Hide resolved
(Related question: should we document how to include files alongside your function like we do for other languages?) |
re-disabling surefire, we should enable but set appropriate defaults so users can run. |
There was a problem hiding this 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
I've tried to trigger profiles (for "only test locally" purposes) based on three things:
@lesv thoughts? May 2020 update: we decided to go with env-var-based profiles, specifically skipping tests when |
* 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
Forgot these in #2394 - cc @averikitsch @grant as FYI.
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!)