Overall code health improvements #187
Conversation
can be removed once GitHub Action CI is generally available
| final Vault vault = container.getVault(NONROOT_TOKEN); | ||
| vault.logical().list("secret/null"); | ||
| List<String> list = vault.logical().list("secret/null"); | ||
| assertEquals(list.size(), 0); |
There was a problem hiding this comment.
@bklawans not sure this is desirable, see #176 (comment)
There was a problem hiding this comment.
Since @steve-perkins bumped the major version and I have fixed up CI. Let's go crazy and break the API 😆
There was a problem hiding this comment.
getListData seems responsible to me as you already have getData but list is not the same as read so perhaps we need a new response class.
| container = new GenericContainer("postgres:11.3-alpine") | ||
| super("postgres:11.3-alpine"); | ||
| this.withNetwork(CONTAINER_NETWORK) | ||
| .withNetworkAliases(hostname) |
There was a problem hiding this comment.
This should fix up the database networking problem the two merges created. @denglertai @steve-perkins
| runCommand("vault", "login", "-ca-cert=" + CONTAINER_CERT_PEMFILE, rootToken); | ||
|
|
||
| runCommand("sh", "-c", "cat <<EOL >> " + CONTAINER_CLIENT_CERT_PEMFILE + "\n" + cert + "\nEOL"); |
There was a problem hiding this comment.
Running on CI against Travis or Github it seems the vault container's uid/gid is the only one with permissions to this file.
There was a problem hiding this comment.
@steve-perkins I would recommend Travis CI for this repo once this PR is merged.
Otherwise, GitHub Action CI will soon be generally available.
There was a problem hiding this comment.
Current CI runs are both available here: https://github.com/casz/vault-java-driver/runs/202654214
|
Hmm, yes, I agree that is not really a good behavior. I can think of two changes - we can still error when a list() call is being made, or modify the list() method to return a structure (ListResults or something similar) with the API status and the results list. I actually like the second idea better, but its a pretty big api change. Let me know which approach you prefer and I’ll create a new PR with the changes.
…-Barry
On Aug 25, 2019, at 7:56 AM, Joseph Petersen ***@***.***> wrote:
@Casz commented on this pull request.
In src/test-integration/java/com/bettercloud/vault/api/LogicalTests.java <#187 (comment)>:
> final Vault vault = container.getVault(NONROOT_TOKEN);
- vault.logical().list("secret/null");
+ List<String> list = vault.logical().list("secret/null");
+ assertEquals(list.size(), 0);
@bklawans <https://github.com/bklawans> not sure this is desirable, see #176 (comment) <#176 (comment)>
In src/test-integration/java/com/bettercloud/vault/util/DbContainer.java <#187 (comment)>:
>
public DbContainer() {
- container = new GenericContainer("postgres:11.3-alpine")
+ super("postgres:11.3-alpine");
+ this.withNetwork(CONTAINER_NETWORK)
+ .withNetworkAliases(hostname)
This should fix up the database networking problem the two merges created. @denglertai <https://github.com/denglertai> @steve-perkins <https://github.com/steve-perkins>
In src/test-integration/java/com/bettercloud/vault/util/VaultContainer.java <#187 (comment)>:
> runCommand("vault", "login", "-ca-cert=" + CONTAINER_CERT_PEMFILE, rootToken);
-
+ runCommand("sh", "-c", "cat <<EOL >> " + CONTAINER_CLIENT_CERT_PEMFILE + "\n" + cert + "\nEOL");
Running on CI against Travis or Github it seems the vault container's uid/gid is the only one with permissions to this file.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#187?email_source=notifications&email_token=AANPQFCFKUDXQCIRTAVLFTLQGKMSLA5CNFSM4IPJHEKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTGHOQ#pullrequestreview-279339962>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANPQFDK4INICBGBFWXEUQTQGKMSLANCNFSM4IPJHEKA>.
|
|
Do you have a preference between enhancing LogicalResponse to have a new method (getListData()) vs creating a new response class that holds the list?
…-Barry
On Aug 25, 2019, at 7:56 AM, Joseph Petersen ***@***.***> wrote:
@Casz commented on this pull request.
In src/test-integration/java/com/bettercloud/vault/api/LogicalTests.java <#187 (comment)>:
> final Vault vault = container.getVault(NONROOT_TOKEN);
- vault.logical().list("secret/null");
+ List<String> list = vault.logical().list("secret/null");
+ assertEquals(list.size(), 0);
@bklawans <https://github.com/bklawans> not sure this is desirable, see #176 (comment) <#176 (comment)>
In src/test-integration/java/com/bettercloud/vault/util/DbContainer.java <#187 (comment)>:
>
public DbContainer() {
- container = new GenericContainer("postgres:11.3-alpine")
+ super("postgres:11.3-alpine");
+ this.withNetwork(CONTAINER_NETWORK)
+ .withNetworkAliases(hostname)
This should fix up the database networking problem the two merges created. @denglertai <https://github.com/denglertai> @steve-perkins <https://github.com/steve-perkins>
In src/test-integration/java/com/bettercloud/vault/util/VaultContainer.java <#187 (comment)>:
> runCommand("vault", "login", "-ca-cert=" + CONTAINER_CERT_PEMFILE, rootToken);
-
+ runCommand("sh", "-c", "cat <<EOL >> " + CONTAINER_CLIENT_CERT_PEMFILE + "\n" + cert + "\nEOL");
Running on CI against Travis or Github it seems the vault container's uid/gid is the only one with permissions to this file.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#187?email_source=notifications&email_token=AANPQFCFKUDXQCIRTAVLFTLQGKMSLA5CNFSM4IPJHEKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTGHOQ#pullrequestreview-279339962>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANPQFDK4INICBGBFWXEUQTQGKMSLANCNFSM4IPJHEKA>.
|
|
If I’m reading the code right, this turned out to be pretty easy. I moved the code that builds the list into LogicalResponse, and it only triggers when the operation is ListV1 or ListV2. I’ve updated all the unit tests and they still pass, and changed the assert in testListExceptionMessageIncludesErrorsReturnedByVault to check for a 404.
…-Barry
On Aug 25, 2019, at 1:56 PM, Joseph Petersen ***@***.***> wrote:
@Casz commented on this pull request.
In src/test-integration/java/com/bettercloud/vault/api/LogicalTests.java <#187 (comment)>:
> final Vault vault = container.getVault(NONROOT_TOKEN);
- vault.logical().list("secret/null");
+ List<String> list = vault.logical().list("secret/null");
+ assertEquals(list.size(), 0);
getListData seems responsible to me as you already have getData but that is not the same as read so perhaps we need a new response class.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#187?email_source=notifications&email_token=AANPQFHGDOXHECZSGMUXJLDQGLWYBA5CNFSM4IPJHEKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTJLPI#discussion_r317414446>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANPQFBPPFWSNS3FZXKGWXTQGLWYBANCNFSM4IPJHEKA>.
|
|
Feel free to send a PR to my branch @bklawans 😓 |
|
@Casz Do you mean casz:chore/overallCodeHealth instead of BetterCloud/master? |
|
Yup |
|
Will do. I'm walking out the door now, but will turn that around this evening (Pacific time zone...) |
|
In no hurry :) |
|
New PR issued to merge my changes to your branch, and I revoked the PR to the main BetterCloud repo. |
…de can be checked. Added new method getListData() to LogicalResult
|
Cheers @bklawans :) |
|
I thought about that, but since the only difference is the return type we can't have both the old and the new method, since Java doesn't consider the return type to be part of the method signature. We will have to rename the new method if we want to keep the old. |
|
Right the will bump to v5 and see compile issues 😊 I can live with that |
|
Fixing the merge conflict 😅 |
|
If you want me to squash/rebase/rewrite history before merging please say so :) |
|
Ehh... the intermingling of |
|
@steve-perkins would you mind enabling Travis so we can have CI on pull requests 😄 |
|
Post-merge, the integration test In context, I assume this has to be a typo. I'll change the expected response to 403. |
|
3231da3 definitely a typo honestly missed that in the merge. |
|
Looking into that now. Virtually all of my recent-enough-to-be-relevant CI experience is with Jenkins, so Travis is a bit unfamiliar. |
|
You just need to enable it. I already added a .travis.yml file for you in this PR. |
|
Incredibly frustrating. I believe that Travis is choking on the fact that this repository is owned by an organization. I've gone through all of the commonly-recommended troubleshooting steps (e.g. making sure that my membership in the organization is set to publicly visible). But the repo is still not available for Travis to use. I'm going to call it a day, and start cutting a release for Maven Central. I may ping Travis support tomorrow. I've also signed up for the beta of GitHub's native CI system. That sign-up request is still pending, but anyone is welcome to write a |
|
Version 5.0.0 has been published, and should appear on Maven Central within the next few hours. |
|
@steve-perkins the PR already added a GitHub CI file: https://github.com/BetterCloud/vault-java-driver/blob/master/.github/workflows/gradle.yml As for you issue enabling Travis, it could be that the project is actually still on So if you head here: https://travis-ci.org/BetterCloud/vault-java-driver you should be able to get it the needed permissions/ Here is the Travis announcement for travis-ci.org moving to travis-ci.com 😓 |
trailing whitespace, import order, final newline, using space instead of tabs.
Had to rewrite most of
SSLUtilsonly to figure out much later it was because Travis/Github CI had issues with whether the container or java had created thesslfolder used for passing files around.For code style, I still have some issues.
The com.bettercloud.vault.json package is mostly written in 2 spacing while the rest is written in 4 spacing.
Personally, I would switch to 4 spacing and 4 continuation spacing instead of 8 for continuation spacing.