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

make DEPLOYMENTS_DIR overridable (#146) #147

Merged
merged 1 commit into from
May 15, 2018

Conversation

vorburger
Copy link
Collaborator

No description provided.

@vorburger
Copy link
Collaborator Author

@rhuss how about this for #146 ?

Copy link
Contributor

@rhuss rhuss 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 with minor nit pick.

If we would have a documentation, we should update it, too. But we don't have one ;-)

@@ -165,6 +165,7 @@ function build_maven() {

echo "=================================================================="
echo "Starting S2I Java Build ....."
mkdir -p ${DEPLOYMENTS_DIR}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please protect this like in

[ -d "${DEPLOYMENTS_DIR}" ] || mkdir -p "${DEPLOYMENTS_DIR}"

The reason is set if we should ever switch to set -e for a more strict error handling, then this would be already safe.

Also dont forget the quotes in case the directory name contains spaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, this comment should go to the original template source. you got the idea ;-)

Copy link
Collaborator Author

@vorburger vorburger May 15, 2018

Choose a reason for hiding this comment

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

Done!

@vorburger
Copy link
Collaborator Author

@rhuss have taken your feedback (hidden) above into account.

PS: The original commit you commented on is actually already merged to master, probably because one of my other PRs you merged today had it as parent? (I've NOT pushed anything myself.) That's why only 1 commit is showing here now, with your suggested improvement to that.

@rhuss
Copy link
Contributor

rhuss commented May 15, 2018

yeah, I rebased a lot yesterday and pushed it to your branches. no worries ;)

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

thankks, looks good.

@rhuss rhuss merged commit 5804783 into fabric8io-images:master May 15, 2018
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.

2 participants