chore(samples): Retail Tutorials. Events (write, rejoin, purge)

[sborisenkox]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

[generated-files-bot]

Warning: This pull request is touching the following templated files:

• .kokoro/build.sh
• samples/pom.xml

[snippet-bot]

Here is the summary of changes.

You are about to add 3 region tags.

• samples/interactive-tutorials/src/main/java/events/PurgeUserEvent.java:17, tag retail_purge_user_event
• samples/interactive-tutorials/src/main/java/events/RejoinUserEvent.java:17, tag retail_rejoin_user_event
• samples/interactive-tutorials/src/main/java/events/WriteUserEvent.java:17, tag retail_write_user_event

---

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

• Refresh this comment

[kurtisvg]

I've only reviewed the first sample. but similar changes should probably be made throughout

[kurtisvg]

In general we usually try to avoid putting secrets here and instead put them in secret-manager.

[kurtisvg]

This issue is still open.

[chingor13]

This is in secret manager. This just configures the env var for ADC

[kurtisvg]

Clients should be re-used and show off a try-with: https://github.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md#client-initialization

[sborisenkox]

Fixed.

[kurtisvg]

Please avoid return types: https://github.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md#method-structure

[tetiana-karasova]

we need to return the operation name to assert it in the test

[kurtisvg]

From the style guide:

Snippet methods should specify a return type of void and avoid returning any value wherever possible. Instead, show the user how to interact with a returned object programmatically by printing some example attributes to the console.

Assert the behavior is correct from the output to console, since that is what the user will use to verify the output is correct.

[chenlei1216]

The comment does not look correct

[sborisenkox]

Fixed.

[chenlei1216]

Comment not correct.

[sborisenkox]

Fixed.

[chenlei1216]

Same here.

[sborisenkox]

Fixed.

Re-requested review.

[kurtisvg]

From the style guide:

Snippet methods should specify a return type of void and avoid returning any value wherever possible. Instead, show the user how to interact with a returned object programmatically by printing some example attributes to the console.

Assert the behavior is correct from the output to console, since that is what the user will use to verify the output is correct.

[kurtisvg]

Why is this included here?

[kurtisvg]

It looks like this is something that is used for testing? It doesn't need to be included in main, the test should set whatever resources it needs on the testing side.

[t-karasova]

the visitor Id is a required value for the writeUserEvent method and is a filter value for the purgeUserEvent method using filtering
So it sud be here

[sborisenkox]

Changed constant visitorId value to random generated.

[kurtisvg]

I think there's some misunderstand about the Method Structure:

The "main" method should just be a collection of inputs the user would need to provide to run the sample. It should use examples that the user can edit and replace with their own data to quickly and easily run the sample. We shouldn't need to rely on it executing for the testing - we should rely directly on the snippet method instead.

It should essentially look like this:

public static void main(String[] args) {
    // TODO(developer): Replace these variables before running the sample.
    String projectId = "my-project-id";
    String defaultCatalog =
        String.format("projects/%s/locations/global/catalogs/default_catalog", projectId);
    // This should be a comment describing where the visitor id is or a link to a different sample if it's created in that sample 
    String visitorId = "my-visitor-id";
   callPurgeUserEvents(visitorId, defaultCatalog);
}

[sborisenkox]

Do you mean projectId should have placeholder value? E.g my-project-id ?
If I do so, the build will fail, and in github CI checks fails also.
visitorId can have any String value, that's okay.

[kurtisvg]

It should have a placeholder value - it's failing because we're using the main for testing instead of calling callPurgeUserEvents directly for testing.

[sborisenkox]

Fixed.

[kurtisvg]

I don't think this needs to be in a separate function - it's only used once and should just be declared inline before it's used.

[sborisenkox]

Fixed, inlined it.

[kurtisvg]

This issue is still open.

[kurtisvg]

Why are we running this with Runtime.getRuntime()? Can't we just call the snippet function directly?

[sborisenkox]

To check the response, we should get output from the console and assert then.
This approach is working.

[kurtisvg]

I think it's better to avoid exec and just capture the stdout directly. Please see this example for the standard way we do it: https://github.com/GoogleCloudPlatform/java-docs-samples/blob/main/storage/cloud-client/src/test/java/com/example/storage/QuickstartSampleIT.java#L47-L67

[t-karasova]

this is the command the user will use to ru the code sample, isn't it better to repeat the user actions here in the test, to simulate the command running and the checking the output?

[kurtisvg]

Typically, main is just a stub to make it easier for the user to provide input. That input should be virtually identical to args needed to run the method directly. In general we try to avoid using environment variables to configure snippets, because it's often easier for users to just edit the sample directly and run in whichever way is most comfortable for them (especially for java, it's not always from a terminal).

[kurtisvg]

This comment is still open.

[sborisenkox]

Changed test logic.

[kurtisvg]

nit: are we sure we only want to show the user starting this operation, and not waiting for the results?

[t-karasova]

This operation is running from several hours up to 3 Days, so we will only show the operation Id as a proof, that it was started

[kurtisvg]

This comment is still open.

[kurtisvg]

It should have a placeholder value - it's failing because we're using the main for testing instead of calling callPurgeUserEvents directly for testing.

[kurtisvg]

marking approved, but please note this comment chain about putting placeholders for the user in "main": #303 (comment)

This is generally a preferred approach because it doesn't encourage users to set environment variables, which can be tricky in some environments.