FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GCF: Add Slack sample + clean up imports by ace-n · Pull Request #2394 · GoogleCloudPlatform/java-docs-samples · GitHub

/ java-docs-samples Public

GCF: Add Slack sample + clean up imports - #2394

Merged
ace-n merged 22 commits into
masterfrom
gcf-slack
Mar 21, 2020
ConversationCommits (22)ChecksFiles changed
Merged

GCF: Add Slack sample + clean up imports#2394
ace-n merged 22 commits into
masterfrom
gcf-slack

Conversation

ace-n commented Mar 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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 requested review from a team, averikitsch and lesv March 13, 2020 06:51
googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 13, 2020
lesv suggested changes Mar 13, 2020
View reviewed changes

lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread functions/snippets/pom.xml
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
ace-n commented Mar 14, 2020
View reviewed changes

ace-n left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated
Comment thread functions/snippets/src/main/java/com/example/functions/SlackSlashCommand.java Outdated

ace-n commented Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

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

lesv commented Mar 16, 2020

Copy link
Copy Markdown
Contributor

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

lesv reviewed Mar 16, 2020
View reviewed changes
Comment thread .kokoro/tests/run_tests.sh
lesv reviewed Mar 16, 2020
View reviewed changes

lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Once you figure out surefire, LGTM

Comment thread functions/snippets/config.slack.json Outdated

ace-n commented Mar 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 reviewed Mar 18, 2020
View reviewed changes
Comment thread functions/snippets/pom.xml
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 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 merged commit 1e5e99a into master Mar 21, 2020
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
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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

averikitsch Awaiting requested review from averikitsch
1 more reviewer
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

Back | FazBrowse Home | New Git URL