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

Split the deploy.sh script to work with both kube & openshift #432

Merged
merged 1 commit into from Oct 2, 2017

Conversation

rthallisey
Copy link
Contributor

These two scripts are very different because one will use OpenShift
templates and the other will use jinja2. I added an intermediate script
that will direct to the proper location.

Changes proposed in this pull request

  • Run make deploy for both openshift and kubernetes envs

@rthallisey rthallisey requested review from jmrodri, abhgupta and djzager and removed request for abhgupta September 13, 2017 17:53
@rthallisey
Copy link
Contributor Author

@rthallisey
Copy link
Contributor Author

rthallisey commented Sep 18, 2017

Testing

The gate uses make deploy for with an OpenShift cluster. Since the gate is green, the other test here is with Kubernetes:

export KUBERNETES=" "
make deploy

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

My only request is this be a single file instead of three.

${VARS} | oc create -f -
if [[ "${KUBERNETES}" ]]; then
echo "Using Kubernetes"
"${BROKER_ROOT}/scripts/kubernetes/deploy.sh"
Copy link
Member

Choose a reason for hiding this comment

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

My recommendation is to bring in the scripts/kubernetes/deploy.sh and scripts/openshift/deploy.sh into this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I didn't want to do is turn these scripts into if k8s then do 100 things else openshift do these 100 things. With two separate files there's a clean separation because these are two separate scripts. So anything kubernetes related I know exists in scripts/kubernetes/.... It's not the best situation, but Bash isn't exactly the best for creating abstraction.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rthallisey +1 keep them separate. They will be easier to manage. Unless they are 100% the same, otherwise it will turn into a bunch of if kube else etc.

These two scripts are very different because one will use OpenShift
templates and the other will use jinja2. I added an intermediate script
that will direct to the proper location.
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

LGTM

${VARS} | oc create -f -
if [[ "${KUBERNETES}" ]]; then
echo "Using Kubernetes"
"${BROKER_ROOT}/scripts/kubernetes/deploy.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

@rthallisey +1 keep them separate. They will be easier to manage. Unless they are 100% the same, otherwise it will turn into a bunch of if kube else etc.

@jmrodri
Copy link
Contributor

jmrodri commented Sep 29, 2017

@djzager -1 to merging the files into one.

@rthallisey rthallisey merged commit 44922f8 into openshift:master Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants