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.

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

deploy scripts support Conjur v5 enterprise #17

Merged
merged 9 commits into from
Aug 2, 2018

Conversation

jtuttle
Copy link
Member

@jtuttle jtuttle commented Aug 1, 2018

Closes #16

@ghost ghost assigned jtuttle Aug 1, 2018
@ghost ghost added the in progress label Aug 1, 2018
README.md Outdated
@@ -10,9 +10,19 @@ environment variables. The setup instructions below walk you through the
necessary steps for configuring your environment and show you which variables
need to be set before deploying.

### Conjur Version

If you are workign with Conjur v4, you will need to set:
Copy link
Contributor

Choose a reason for hiding this comment

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

little typo here: workign

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

env:
# Only required for Conjur v5
- name: CONJUR_DATA_KEY
value: "{{ DATA_KEY }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to call this var CONJUR_DATA_KEY as well? more explicit when you see it in scripts, i think

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, that works. Will change it.

@jtuttle jtuttle force-pushed the 16--deploy-scripts-support-conjur-v5-enterprise branch from 7439e46 to a518c45 Compare August 1, 2018 15:30
@jtuttle jtuttle force-pushed the 16--deploy-scripts-support-conjur-v5-enterprise branch from a518c45 to 29e175f Compare August 1, 2018 15:31
@dustinmm80
Copy link
Contributor

dustinmm80 commented Aug 1, 2018

We need a few more things before we can merge:

  • default CONJUR_VERSION to CONJUR_MAJOR_VERSION, since this is what summon-conjur uses
  • fix Jenkinsfile conflict in this PR
  • fix Jenkins build

Once these are done we're all set to squash/merge.

@@ -6,10 +6,16 @@ set -euo pipefail
set_namespace $CONJUR_NAMESPACE_NAME

if [ $PLATFORM = 'kubernetes' ]; then
ui_url="https://$(get_master_service_ip):443"
ui_url="https://$(get_master_service_ip):443"
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to add 443 on the end here? if it's https:// prefix it's 443 by default. Looks a bit weird in output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, will fix.

Copy link
Contributor

@dustinmm80 dustinmm80 left a comment

Choose a reason for hiding this comment

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

Looks good, merging!

@dustinmm80 dustinmm80 merged commit 151dd1d into master Aug 2, 2018
@ghost ghost removed the in progress label Aug 2, 2018
@dustinmm80 dustinmm80 deleted the 16--deploy-scripts-support-conjur-v5-enterprise branch August 2, 2018 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants