Esilky handle ag client startup problems#377

[esilky]

• Fixes the playing of local audio files
• Improves handling of start-up problems
• Plays audio messages for initial start-up problems (for clients where the console and log file are not visible or easily accessible
• Fixes an error in the log4j configuration
• Improves some of the logging messages
• Adds tests for playing local audio and reading-processing the configure properties file by Driver (main class)
• Adds a .gitignore for JAR files in the root of the project folder (to make it easier to run locally)
• Update a Log4J2 configuration property name

[esilky]

@erezbi @arivatibm The change to README.md is part of the other pull request (updates to README based on review (branch esilky-agc-readme-2018-05) created earlier today). I'm not sure if it should be included in this one as well? If not, I can try to remove it.

[arivatibm]

if it's a duplicate pull request then you can close it   Regards,   Ari Volcoff Research Staff Member Watson Assistant Solutions , IoT And Wearables IBM - Haifa Research Lab  
Mobile: +972-58-4168357 | Phone: +972-4-8281319 E-mail: ARIV@il.ibm.com LinkedIn: https://il.linkedin.com/in/avolcoff
    ----- Original message -----From: Ed Silky notifications@github.comTo: Watson-Personal-Assistant/AudioClientSampleCodeJava AudioClientSampleCodeJava@noreply.github.comCc: arivatibm ariv@il.ibm.com, Mention mention@noreply.github.comSubject: Re: [Watson-Personal-Assistant/AudioClientSampleCodeJava] Esilky handle ag client startup problems#377 (#3)Date: Tue, May 29, 2018 5:02 AM  @erezbi @arivatibm The change to README.md is part of the other pull request (updates to README based on review (branch esilky-agc-readme-2018-05) created earlier today). I'm not sure if it should be included in this one as well? If not, I can try to remove it. —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
 

[esilky]

I would prefer to change the name from "IAMAPIKey" to "keyIAMAPI" based on Java standard naming practices. If you are strongly in favor of keeping it as-is "IAMAPIKey", I will change it back. It will make it harder for core Java developers to identify it as a property (rather than a constant).

[esilky]

@arivatibm - please take a look at my responses re: name change of "IAMAPIKey" to "keyIAMAPI". Let me know what you think. I would like to get this merged to clear changes out of my workspace. If you feel strongly about keeping it "IAMAPIKey" I will change it back.

[arivatibm]

@esilky - Please change to "iamApiKey "

[esilky]

@arivatibm - updated the name based on your review and comments.

[esilky]

@erezbi @arivatibm - Is there anything else I need for you to approve and merge this? I'm sorry if I missed something.

[arivatibm]

@esilky we need QA to test it before we can merge I've asked Ramesh to test it

[erezbi]

@esilky - Ramesh is waiting for you. He understood from you that you still need to fix something in this PR, so he will test it afterwards.

Please notify us when you fix this, so that we can test and merge this.

[esilky]

Okay - not sure what. I'll check with him about it.

[esilky]

@erezbi I can't find Ramesh's GitHub ID to include him in the comment. I sent him an email. I'm not sure what it is waiting on.

[esilky]

@ramupadh after our meeting, do you have what you need to be able to test this? (@erezbi fyi - Ramesh and I met for some time today on what this does and how to test it)

[arivatibm]

Ramesh mentioned that: Tested with following scenario working as per requriement. 1)No configuration properties Acutal Result: The configure.properties file should be in the 'config' folder. with audio. 2)Empty configuration properties Message saying improper configuration properties with audio.

3)valid value in configuration properties but invalid iamkey message saying "unable to connect server." But audio is not saying anything on the issue. rasing sepearte issue on these scenario. 4)Valid Configuration properties Giving valid response.

so @esilky needs to fix the 3rd issue before we can merge @erezbi FYI

[esilky]

In case #3, the response is "'401' - Unauthorized". Currently the client main retries, but that should probably be a fatal error - since it's not going to succeed unless the configure.properties is changed or the client ID is added to the server.

I will look at that and make the appropriate changes to fail out of the client and play an appropriate audio response.

[esilky]

@ramupadh - I made the change to play an audio error message when there is an authentication error and to exit rather than retry (since it isn't something the client can recover from without changing the configuration).

[erezbi]

@rameshupadhyaper Can you please verify this again. Make sure that the last changes didn't break anything. Make sure that issue '3' was solved according to @esilky 's description - error message played to the user, and no retry.

FYI @arivatibm

[rameshupadhyaper]

Tested with following scenario working as per requirement. 1)No configuration properties Actual Result: The configure. Properties file not valid. with audio. 2)Empty configuration properties Message saying improper configuration properties with audio. 3)Valid Configuration properties Giving valid response. perquisite: Jar file created and then made change in configuration file. 4)After i made change in valid configuration like invalid IAMKEY Ans:Still give valid response for some intent :Hi====>Hello what time is it ===>its 18.04 Expected: It should say invalid configuration.properties, but it give response.

5) Deleted jar and add invalid configure file.Then create jar start the client: java -Dlog4j2.configurationFile=config/log4j2.xml -jar wpa-1.4-SNAPSHOT.jar Answer: Audio message says " There is error authentication with server". Whether scenario 4 is valid?

[erezbi]

@rameshupadhyaper thanks. So I understand that everything worked apart from the test where you gave a bad IAM ID and it still responded to you?

[rameshupadhyaper]

@erezbi : yes , if I create jar first then changes configure file. Still gives response.

[esilky]

@rameshupadhya - are you sure that the process was stopped when you tried running with the invalid IAM ID? I don't know how it would be possible for it to work with invalid values in the config.properties if it is starting anew. Also, you shouldn't need the '-D' parameter in the start up. It should pick up the correct location for the log4j configuration file unless you want to put it someplace other than the config directory.

[erezbi]

After conversation with @esilky , he tested 4 and 5 again, and they work. We think that the IAM Key was changed while the client was running. @rameshupadhyaper can you please try this one more time?

[rameshupadhyaper]

@esilky Please confirm the scenario which I encountered is valid or invalid. Prerequisite: Valid IAM key in configuration file, then I will create jar using mvn package. I will start controller with command java -Dlog4j2.configurationFile=config/log4j2.xml -jar wpa-1.4-SNAPSHOT.jar

steps: 1) Change the configuration file with invalid IAM key. 2) Again use just converse by entering Hi ==>Hello It gives response.

Another scenario: After step2 , I will stop the controller again stars with command java -Dlog4j2.configurationFile=config/log4j2.xml -jar wpa-1.4-SNAPSHOT.jar Expected: Audio message should say. Actual: it only displays 401 unauthorized.

[esilky]

@rameshupadhyaper Can we set up a meeting for you to go through this so I can see it?

[esilky]

@rameshupadhyaper - I went through this with a clean directory and it worked for me. This is what I did:

Set up for testing (get the latest code for the branch in the PR)... Create a 'test' directory, change to it, clone the client, checkout the branch, fetch it. mkdir test cd test git clone git@github.com:Watson-Personal-Assistant/AudioClientSampleCodeJava.git git checkout remotes/origin/esilky--handle-AGClient-startup-problems#377 git fetch

Run the Maven build. mvn package

Copy the resulting JAR to the root of the project directory. cp target/wpa-1.4-SNAPSHOT.jar .

Configure the configure.properties with valid values. Test by running: java -Dlog4j2.configurationFile=config/log4j2.xml -jar wpa-1.4-SNAPSHOT.jar

Stop the client using ^C in the terminal window. In config/configure.properties change the IAMAPIKey to an invalid key value. Test: java -Dlog4j2.configurationFile=config/log4j2.xml -jar wpa-1.4-SNAPSHOT.jar

There should be an audio response: "There was an error authenticating with the server."

[rameshupadhyaper]

@esilky @erezbi Tested with latest code base: esilky--handle-AGClient-startup-problems#377. Working as per requirement. audio response says "There was an error authenticating with the server."

[erezbi]

@esilky please stop submitting changes to this PR. @arivatibm please review latest three commits so that we can merge this.