Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enterprise tests exist for Java client library #30

Merged
merged 3 commits into from
May 31, 2019
Merged

Enterprise tests exist for Java client library #30

merged 3 commits into from
May 31, 2019

Conversation

sigalsax
Copy link
Contributor

Currently, the Java Client tests against the OSS, but not for Enterprise (DAP). This PR adds tests that run against the appliance (EE/DAP).

Connected to #29

@sigalsax sigalsax self-assigned this May 13, 2019
izgeri
izgeri reviewed May 14, 2019
View reviewed changes
test.sh Outdated Show resolved Hide resolved
izgeri
izgeri reviewed May 14, 2019
View reviewed changes
test.sh Outdated Show resolved Hide resolved
izgeri
izgeri reviewed May 14, 2019
View reviewed changes
test.sh Show resolved Hide resolved
izgeri
izgeri reviewed May 14, 2019
View reviewed changes
test.sh Outdated
@@ -18,18 +23,66 @@ function main() {
runHTTPSTests
Copy link
Contributor

Choose a reason for hiding this comment

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

finish should probably be called at the end of this to tear down the OSS containers

izgeri
izgeri reviewed May 14, 2019
View reviewed changes
test.sh Outdated Show resolved Hide resolved
@sigalsax sigalsax force-pushed the 29-ee-library-testing branch 3 times, most recently from df9d76b to f04bf89 Compare May 20, 2019 12:30
izgeri
izgeri reviewed May 20, 2019
View reviewed changes
docker-compose.yml Outdated Show resolved Hide resolved
izgeri
izgeri reviewed May 20, 2019
View reviewed changes
docker-compose.yml Outdated Show resolved Hide resolved
izgeri
izgeri reviewed May 20, 2019
View reviewed changes
test.sh Outdated Show resolved Hide resolved
izgeri
izgeri reviewed May 20, 2019
View reviewed changes
test.sh Outdated
echo '------------------------------------------------------------'
docker-compose down -v
function main() {
# runOSS
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be uncommented before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

izgeri
izgeri reviewed May 20, 2019
View reviewed changes
test.sh Outdated Show resolved Hide resolved
izgeri
izgeri reviewed May 20, 2019
View reviewed changes
test.sh Outdated Show resolved Hide resolved
izgeri
izgeri reviewed May 20, 2019
View reviewed changes
test.sh Outdated Show resolved Hide resolved
sgnn7
sgnn7 suggested changes May 20, 2019
View reviewed changes
Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

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

@sigalsax left some comments here. since the code wasn't fully done yet, it should have been opened as "draft" next time.

docker-compose.yml Outdated
- "8080:80"
- "8443:443"
- "8080:80"
- "8443:443"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should try using ports >1024 (8080 and 8443 can be fine here) for local bindings as we talked on Slack (https://www.w3.org/Daemon/User/Installation/PrivilegedPorts.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there another port commonly used port I could use (instead of 8443)? B/c running the scripts together, causes a conflict b/c nginx also needs 8443.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not, I could just tear down the containers after every suite of tests run

docker-compose.yml Outdated
cuke-master:
image: registry2.itci.conjur.net/conjur-appliance-cuke-master:5.0-stable
ports:
- "443:443"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on ports needing probably to be over 1024

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
test.sh Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated
@@ -57,7 +144,7 @@ function loadTestPolicy() {
/bin/bash -c "conjur policy load root /tmp/test-policy/root.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as elsewhere: is wrapping a command in bash -c needed?

test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Show resolved Hide resolved
@sigalsax sigalsax force-pushed the 29-ee-library-testing branch 6 times, most recently from ecdf7be to 06afe13 Compare May 22, 2019 13:32
@sigalsax sigalsax force-pushed the 29-ee-library-testing branch 2 times, most recently from 22c173e to a04a920 Compare May 28, 2019 20:20
@sigalsax sigalsax force-pushed the 29-ee-library-testing branch 3 times, most recently from 1a666c7 to 71e7f8e Compare May 29, 2019 11:59
@sigalsax
Add enterprise testing containers and configurations
897f34d 
This adds enterprise containers and enterprise tests and updates configurations and versioning
@sigalsax sigalsax force-pushed the 29-ee-library-testing branch 11 times, most recently from 4c0b15a to 40a821a Compare May 30, 2019 19:53
@sigalsax sigalsax requested review from sgnn7 and izgeri May 30, 2019 19:56
@sigalsax
Copy link
Contributor Author

I am going to go back and separately create a CONTRIBUTING.md much like we do for the secretless repo

@sigalsax sigalsax changed the title WIP: Enterprise tests exist for Java client library Enterprise tests exist for Java client library May 31, 2019
@sigalsax
Update JAVA_HOME path to fix failing tests
f332619 
Previously, a new keystore was (unintentionally) being created but the default keystore was not being updated so there was no way of knowing where to grab the cert. This fixes the path issue
@sigalsax sigalsax force-pushed the 29-ee-library-testing branch from 40a821a to f332619 Compare May 31, 2019 13:43
sgnn7
sgnn7 suggested changes May 31, 2019
View reviewed changes
Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

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

@sigalsax Just a few more comments but it's looking much better!

test.sh Show resolved Hide resolved
test.sh
echo 'Removing test environment'
echo '------------------------------------------------------------'
docker-compose down -v
function main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't see cleanup before the start of the tests

test.sh Outdated
}

# Run DAP Enterprise test suite
function runDAP() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally didn't notice this before and you don't have to fix it for this PR but Bash function names use camel_case captialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it (:

test.sh Outdated
docker exec ${dap_client_cid} /bin/bash -c "$exec_command"

echo "convert pem to der file and copy it to share memory"
convert_command="openssl x509 -outform der -in /root/conjur-cucumber.pem -out /test-cert/conjur-cucumber.der"
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to split this command into multiple lines as well since it's too long

test.sh

echo "convert pem to der file and copy it to share memory"
convert_command="openssl x509 -outform der -in /root/conjur-cucumber.pem -out /test-cert/conjur-cucumber.der"
docker exec ${dap_client_cid} ${convert_command}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still relevant for the new code added - don't worry about the old code unless you want to fix that up too.

@sigalsax sigalsax force-pushed the 29-ee-library-testing branch from 5e81daf to b232a34 Compare May 31, 2019 17:21
sgnn7
sgnn7 approved these changes May 31, 2019
View reviewed changes
Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

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

@sigalsax LGTM!

@sgnn7 sgnn7 merged commit 8f5a6d2 into master May 31, 2019
@sgnn7 sgnn7 deleted the 29-ee-library-testing branch May 31, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgnn7 sgnn7 sgnn7 approved these changes

@izgeri izgeri Awaiting requested review from izgeri

Assignees

@sigalsax sigalsax

Labels
component/api/java implemented
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sigalsax @sgnn7 @izgeri