search

cyberark / conjur-api-java

Java client for the CyberArk Conjur API
Apache License 2.0
17 stars 14 forks source link

Review technical debt in repo and make plan for nearterm enhancements #63

Closed izgeri closed 4 years ago

izgeri commented 4 years ago

Review this repo and document items in the following categories with links to filed issues.

In addition, document succinctly whether this integration works with Conjur OSS / DAP and what methods are supported.

Supported flows

Note which flows are supported, and next to each that is supported note the method name. We can use this to improve the README documentation.

Methods

Create a Conjur Instance

Set/Retrieve Secrets:

Improvements to release process

Repo documentation improvements

Test suite improvements

The test suite is difficult to improve without just adding more tech debt. The tests are all integration tests that take a long time to run.

Unit tests

This codebase has 0 unit tests which are critical for quick cycles and high quality of the codebase. Codebase needs a lot more modularization to serve as a good base for unit tests.

Integration tests

Slow and old - these can probably be improved

Repo standard maintenance tasks

Enhancement requests to consider

Note: Includes items from sections above this one

Bugs

N/A (@sgnn7)

izgeri commented 4 years ago

@sgnn7 Jake is covering the feature review for this project, but asked to get help on the tech debt review. Can you weigh in on that? It's fine with me if @JakeQuilty gets to a point where his piece is "done" and the card moves back to "ready" until you're ready to pull it - it's unusual to do it this way, but it doesn't make sense to me to split this card into two (lmk if you disagree, though)

JakeQuilty commented 4 years ago

Review this repo and document items in the following categories with links to filed issues.

In addition, document succinctly whether this integration works with Conjur OSS / DAP and what methods are supported.

Supported flows

Note which flows are supported, and next to each that is supported note the method name. We can use this to improve the README documentation.

Methods

Create a Conjur Instance

Conjur()

Conjur(String username, String password)

Conjur(String username, String password, String authnUrl)

Conjur(Token token)

Conjur(Credentials credentials)

Set/Retrieve Secrets:

conjur.variables().retrieveSecret(variableID) conjur.variables().addSecret(variableID, String secret)

Improvements to release process

Repo documentation improvements

In the sample policy the host is labeled as:

- !policy
  id: <POLICY_ID>
  body:
    - !host
      id: <NAME_OF_HOST>

But later on the guide says: For example, the host/, not just . This was extremely confusing, because it makes it seem like CONJUR_AUTHN_LOGIN should be just: host/<NAME_OF_HOST(in the yaml)> but it's really: host/POLICY_ID/NAME_OF_HOST

The MacOS path to the JRE_HOME in the last step is /Library/Java/JavaVirtualMachines/jdk1.8.0_251.jdk/Contents/Home/jre

Test suite improvements (in particular, tests to add to alert us early to breakages)

The test suite is difficult to improve without just adding more tech debt. The tests are all integration tests that take a long time to run. These aren't too flexible to write either, because you need API keys for the users. Since there is no way to retrieve an API key in the API you have to get them from the test.sh script and then set them as environment variables and then get them from inside the API. The repo could really benefit from unit tests, but all of the methods and classes are so linked together that there's not really any room write a unit test. I think the test suite just needs redone after a major overhaul to core of the project that would allow for testing.

Repo standard maintenance tasks

Enhancement requests to consider

Adding policy handling (cyberark/conjur-api-java#68) Adding listing of resources (cyberark/conjur-api-java#69)

Bugs

I got this error when running the Maven build with JDK 13 and 14:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.641 s
[INFO] Finished at: 2020-04-30T22:56:39-04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.2.0:javadoc (default) on project conjur-api: An error has occurred in Javadoc report generation: 
[ERROR] Exit code: 1 - javadoc: error - The code being documented uses modules but the packages defined in https://docs.oracle.com/javase/8/docs/api/ are in the unnamed module.
[ERROR] 
[ERROR] Command line was: /Library/Java/JavaVirtualMachines/jdk-13.0.1.jdk/Contents/Home/bin/javadoc @options @packages
[ERROR] 
[ERROR] Refer to the generated Javadoc files in '/path/path/path/path/conjur-api-java/target/site/apidocs' dir.
[ERROR] 
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
sgnn7 commented 4 years ago

@izgeri I updated the master comment here with my findings too. Moving to review column.