Java edits for kb-sdk 1.3 #350

ialarmedalien · 2020-06-11T15:15:19Z

Some changes to prepare kb-sdk for generating GitHub actions and updating the default module builds.

The first commit is all whitespace changes -- my editor removes trailing whitespace and turns tabs into spaces, so I committed all the whitespace changes prior to the content changes. I would suggest viewing just the last three commits.

The changes in this PR comprise:

adding a build date stamp to kb-sdk version
cleaning up some commented-out lines in the Module Builder
added in some extra feedback to alert users as to whether auth is using an environment variable or an auth token in a file
added in a GitHub action that does some rudimentary checks of kb-sdk functioning, in lieu of having proper tests.

…h spaces

ialarmedalien · 2020-06-11T15:16:16Z

build.xml

@@ -102,7 +102,7 @@
  </target>


-
+  <tstamp/>


ialarmedalien · 2020-06-11T15:16:41Z

build.xml

@@ -116,7 +116,8 @@
    <echo file="${src}/us/kbase/mobu/git.properties">### PLEASE DO NOT CHANGE THIS FILE MANUALLY! ###
 giturl=${git.url}
 branch=${git.branch}
-commit=${git.commit}</echo>
+commit=${git.commit}
+build_timestamp=${DSTAMP}-${TSTAMP}</echo>


add date and time to the git.properties file

ialarmedalien · 2020-06-11T15:17:18Z

src/java/us/kbase/mobu/ModuleBuilder.java

+            + (gitCommit == null ? "" : (" (commit " + gitCommit + ")")));
+    }
+
+    public static String getBuildTimestamp() {


Read the build timestamp from the git.properties file

ialarmedalien · 2020-06-11T15:17:59Z

src/java/us/kbase/mobu/ModuleBuilder.java

+        String timestamp = getBuildTimestamp();
+        System.out.println("KBase SDK version " + VERSION
+            + (timestamp == null ? "" : ("_" + timestamp))
+            + (gitCommit == null ? "" : (" (commit " + gitCommit + ")")));


Include the build date and time with the kb-sdk version.

The version string now looks like this:
KBase SDK version 1.2.0_20200611-1439 (commit 7e58f55ad589fa64f83804c2dc9e380620210c45) (from GitHub Actions output)

ialarmedalien · 2020-06-11T15:18:28Z

src/java/us/kbase/mobu/initializer/ModuleInitializer.java

@@ -169,19 +169,16 @@ public void initialize(boolean example) throws Exception {
        }

        switch(this.language) {
-            // Perl just needs an impl file and a start server script
+            // start_server scripts are generated by the Makefile


clean up comments and remove commented-out lines

ialarmedalien · 2020-06-11T15:18:53Z

src/java/us/kbase/mobu/initializer/ModuleInitializer.java

-                    false, // core or sync clients
-                    false, // dynamic client
-                    null, //tagVer
+                    true,   // async clients


whitespace changes for nicer alignment

ialarmedalien · 2020-06-11T15:20:07Z

src/java/us/kbase/mobu/tester/ConfigLoader.java

@@ -38,6 +38,11 @@ public ConfigLoader(Properties props, boolean testMode,
        if (authUrl == null)
            throw new IllegalStateException("Error: 'auth_service_url' parameter is not set in " +
                    configPathInfo);
+        endPoint = props.getProperty("kbase_endpoint");


moved this check up so that kb-sdk will fail earlier if the kbase_endpoint property is not set

src/java/us/kbase/mobu/tester/ConfigLoader.java

ialarmedalien · 2020-06-11T15:22:28Z

src/java/us/kbase/mobu/tester/ModuleTester.java

+                callbackUrl == null
+                  ? "http://fakecallbackurl"
+                  : callbackUrl.toExternalForm()
+            ).exec(tlDir).getExitCode();


Whitespace changes only -- an upcoming PR has changes to this command and this reformatting makes them easier to understand.

ialarmedalien · 2020-06-11T15:22:41Z

src/java/us/kbase/mobu/util/ProcessHelper.java

-                Runtime.getRuntime().exec(cmd.cmdParts, null, workDir);
+        process = cmd.cmdLine != null
+          ? Runtime.getRuntime().exec(cmd.cmdLine, null, workDir)
+          : Runtime.getRuntime().exec(cmd.cmdParts, null, workDir);


whitespace changes only.

MrCreosote

This seems mostly innocuous and it's good to have some automated testing even if it doesn't test most of the code base.

That being said, this is critical infrastructure and making changes without having a real test suite running makes me very nervous.

src/java/us/kbase/mobu/ModuleBuilder.java

.github/workflows/kb_sdk-actions.yml

src/java/us/kbase/mobu/tester/ConfigLoader.java

…trieval of information from git.properties file.

…tarting kb-sdk. Add release notes. Fix KBase report class name error in Java version.

ialarmedalien · 2020-06-16T16:43:42Z

src/java/us/kbase/templates/module_java_impl.vm.properties

@@ -22,7 +22,7 @@ import assemblyutil.GetAssemblyParams;
 import assemblyutil.SaveAssemblyParams;
 import kbasereport.CreateParams;
 import kbasereport.KBaseReportClient;
-import kbasereport.Report;
+import kbasereport.SimpleReport;


Java bug fix - incorrect class name.

ialarmedalien · 2020-06-16T16:50:11Z

GitHub Actions results for commit 9ac1aba: https://github.com/ialarmedalien/kb_sdk/actions/runs/137434654

.github/workflows/kb_sdk-actions.yml

MrCreosote · 2020-06-17T17:04:44Z

src/java/us/kbase/mobu/ModuleBuilder.java

-        return gitCommit;
+            propertyValue = gitProps.getProperty(propertyToGet);
+        } catch (Exception e) {
+            showError("Error while retrieving version information", e.getMessage());


showError just prints the error, it doesn't exit. I think you'll have to do something else here to stop execution.

I don't think it's necessary to stop execution if the git.properties file can't be found or these properties aren't available -- it's basically just adding in some extra "nice to have" information, it isn't mission critical. The properties file is generated during the build process and deleting it afterwards had no effect on the functioning of kb-sdk.

But why not? If it's not there, clearly something went wrong with the build. Reduce runtime variables IMO.

My thinking is more along the lines of "I don't want to give kb-sdk a reason to die unexpectedly", plus I couldn't create a situation manually where kb-sdk built successfully but the git.properties file was not present.

I'm thinking I don't want unexpected behavior from the SDK when something's broken. I want it to fail fast.

plus I couldn't create a situation manually where kb-sdk built successfully but the git.properties file was not present.

Then failing won't cause any problems in practice unless something is really wrong, so might as well fail.

OK, added a System.exit(1); after emitting the error message.

.github/workflows/kb_sdk-actions.yml

MrCreosote · 2020-06-17T17:21:33Z

.github/workflows/kb_sdk-actions.yml

+      fail-fast: false
+      matrix:
+        os: [ubuntu-16.04, ubuntu-18.04, ubuntu-20.04]
+        app: [WsLargeDataIO, KBaseReport]


Hmm... this means if those repos break then kb-sdk test also breaks. I understand why you're doing this but making the sdk build depend on outside repos building and testing correctly seems overly fragile.

Also, neither of these repos test callback server functionality

What would you suggest here? The tests are kind of moot anyway because the app base images may have a different version of kb-sdk on them, so the build/compile step won't really be testing the revised kb-sdk. There should be something to test that the changes aren't going to break existing repos.

Well personally, I wouldn't test against outside repos and I would depend on a good suite of regression tests and quality PR reviews to ensure that changes don't break repos. So let's just make that happen yeah

Hah! I have some ideas for setting up some better regression tests but I didn't want to put too much into this PR, especially since I am working around the knowledge that kb-sdk v1.1.0 to 1.2.0 introduces breaking changes in the python code which have been masked because the sdkbase repo has not been updated. 🤦

I really wish that the old testing mechanisms had not been disabled!

Were they actually disabled? I just thought that no one could get them to run except me. I'm not even sure if I can get them to run at this point, so I wouldn't be surprised if bitrot had set in.

I tried to get them going a while back, but they ran on a VM which has now disappeared, and the config/set up files were removed from the repo.

the config/set up files were removed from the repo

That's just... wow.

.github/workflows/kb_sdk-actions.yml

RELEASE_NOTES.txt

src/java/us/kbase/mobu/tester/ConfigLoader.java

MrCreosote · 2020-06-17T20:45:52Z

Java client generated by kb-sdk uses the correct name for KBase report

This still seems off. The Java client doesn't know about the KBase report at all in general. The generated tests for the java module were using an incorrect class name.

ialarmedalien · 2020-06-17T23:15:30Z

Java client generated by kb-sdk uses the correct name for KBase report

This still seems off. The Java client doesn't know about the KBase report at all in general. The generated tests for the java module were using an incorrect class name.

"Corrected a class name in the Java client generated by kb-sdk"?

MrCreosote · 2020-06-17T23:32:36Z

I would say 'Corrected a class name in the generated tests for the Java SDK module'. It doesn't really have anything to do with the Java client generally.

If you want to be really specific then 'Corrected a class name for the Java KBaseReport client in the generated tests for the Java SDK module'

Let's skip ci this time.

MrCreosote · 2020-06-19T17:09:52Z

src/java/us/kbase/mobu/ModuleBuilder.java

+            propertyValue = gitProps.getProperty(propertyToGet);
+        } catch (Exception e) {
+            showError("Error while retrieving version information from git.properties file: ", e.getMessage());
+            System.exit(1);


When I was looking at the module builder code earlier, I noticed that most of the methods returned the return code rather than calling exit() themselves. Does that need to happen here? I don't actually understand why they were passing back the return code, but I assume it's for some good reason.

Maybe testability? If you return the code, you can test the method, but not if you call exit().

This happens before any of the module builder methods are called, so you have to just exit. The module builder methods all expect an integer (representing success or otherwise) to be returned:

public static void main(String[] args) throws Exception { // code adding the various valid commands and args to JCommander // ModuleBuilder line 97: print name and version printVersion(); // parse the arguments and gracefully catch any errors try { jc.parse(args); } catch (RuntimeException e) { showError("Command Line Argument Error", e.getMessage()); System.exit(1); } // snip some help command stuff // if we get here, we have a valid command, so process it and do stuff int returnCode = 0; if(jc.getParsedCommand().equals(HELP_COMMAND)) { showCommandUsage(jc,help,System.out); } else if(jc.getParsedCommand().equals(INIT_COMMAND)) { returnCode = runInitCommand(initArgs,jc); // etc.

MrCreosote

Approving with trepidation as the test suite can't be run.

ialarmedalien added 4 commits June 11, 2020 07:18

Whitespace changes only: removing trailing spaces, replacing tabs wit…

35e7479

…h spaces

Typo in error message

8d0e981

Adding build date/time to kb-sdk. Failing earlier in kb-sdk test.

2a7474a

Adding actions file

7e58f55

ialarmedalien commented Jun 11, 2020

View reviewed changes

build.xml

@@ -102,7 +102,7 @@

</target>

<tstamp/>

Copy link

Collaborator Author

ialarmedalien Jun 11, 2020

Choose a reason for hiding this comment

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

Timestamp

ialarmedalien commented Jun 11, 2020

View reviewed changes

src/java/us/kbase/mobu/tester/ConfigLoader.java Show resolved Hide resolved

ialarmedalien commented Jun 11, 2020

View reviewed changes

ialarmedalien marked this pull request as ready for review June 11, 2020 15:23

MrCreosote requested changes Jun 11, 2020

View reviewed changes

src/java/us/kbase/mobu/ModuleBuilder.java Outdated Show resolved Hide resolved

.github/workflows/kb_sdk-actions.yml Outdated Show resolved Hide resolved

src/java/us/kbase/mobu/tester/ConfigLoader.java Show resolved Hide resolved

ialarmedalien added 2 commits June 16, 2020 09:36

Adding in Java example generate/test to github actions; DRYing out re…

835b9cc

…trieval of information from git.properties file.

Updating actions to create a build/test matrix. Report version when s…

9ac1aba

…tarting kb-sdk. Add release notes. Fix KBase report class name error in Java version.

ialarmedalien force-pushed the java_edits_for_kb_sdk_1.3 branch from a13df0a to 9ac1aba Compare June 16, 2020 16:36

ialarmedalien commented Jun 16, 2020

View reviewed changes

ialarmedalien requested a review from MrCreosote June 16, 2020 18:42

MrCreosote requested changes Jun 17, 2020

View reviewed changes

Updating release notes with more specific details

2384e3b

ialarmedalien added 2 commits June 17, 2020 16:40

Reworded release notes message.

48f8e29

Let's skip ci this time.

Die if the git.properties file cannot be found

c3e81fb

ialarmedalien requested a review from MrCreosote June 18, 2020 20:14

MrCreosote reviewed Jun 19, 2020

View reviewed changes

MrCreosote approved these changes Jun 19, 2020

View reviewed changes

MrCreosote merged commit ad5e402 into kbase:develop Jun 19, 2020

ialarmedalien deleted the java_edits_for_kb_sdk_1.3 branch June 19, 2020 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java edits for kb-sdk 1.3 #350

Java edits for kb-sdk 1.3 #350

ialarmedalien commented Jun 11, 2020 •

edited

Loading

ialarmedalien Jun 11, 2020

ialarmedalien Jun 11, 2020

ialarmedalien Jun 11, 2020

ialarmedalien Jun 11, 2020 •

edited

Loading

ialarmedalien Jun 11, 2020

ialarmedalien Jun 11, 2020

ialarmedalien Jun 11, 2020

ialarmedalien Jun 11, 2020

ialarmedalien Jun 11, 2020

MrCreosote left a comment

ialarmedalien Jun 16, 2020

ialarmedalien commented Jun 16, 2020

MrCreosote Jun 17, 2020

ialarmedalien Jun 17, 2020

MrCreosote Jun 17, 2020

ialarmedalien Jun 17, 2020

MrCreosote Jun 17, 2020

ialarmedalien Jun 18, 2020 •

edited

Loading

MrCreosote Jun 17, 2020

MrCreosote Jun 17, 2020

ialarmedalien Jun 17, 2020

MrCreosote Jun 17, 2020

ialarmedalien Jun 17, 2020

MrCreosote Jun 17, 2020

ialarmedalien Jun 17, 2020

MrCreosote Jun 17, 2020

MrCreosote commented Jun 17, 2020

ialarmedalien commented Jun 17, 2020 •

edited

Loading

MrCreosote commented Jun 17, 2020

MrCreosote Jun 19, 2020

MrCreosote Jun 19, 2020

ialarmedalien Jun 19, 2020 •

edited

Loading

MrCreosote left a comment

Java edits for kb-sdk 1.3 #350

Java edits for kb-sdk 1.3 #350

Conversation

ialarmedalien commented Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialarmedalien Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCreosote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialarmedalien commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialarmedalien Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCreosote commented Jun 17, 2020

ialarmedalien commented Jun 17, 2020 • edited Loading

MrCreosote commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialarmedalien Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

MrCreosote left a comment

Choose a reason for hiding this comment

ialarmedalien commented Jun 11, 2020 •

edited

Loading

ialarmedalien Jun 11, 2020 •

edited

Loading

ialarmedalien Jun 18, 2020 •

edited

Loading

ialarmedalien commented Jun 17, 2020 •

edited

Loading

ialarmedalien Jun 19, 2020 •

edited

Loading