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

script: Added support for Mac user for using docker compose script #409

Closed
wants to merge 4 commits into from
Closed

script: Added support for Mac user for using docker compose script #409

wants to merge 4 commits into from

Conversation

MPP-SG
Copy link

@MPP-SG MPP-SG commented Sep 16, 2020

What was the problem

This pull request originated from my opened issue #407 .
As a Mac user I couldn't use the provided script ./bin/docker-compose up -d because on Darwin (Mac) the command readlink is not supported

Summary about the code changes

I added support for Mac user for useing the command ./bin/docker-compose up -d

This is done via:

  1. Check the host machine (either Linux or Darwin or NOTSUPPORTED)
  2. Set the variable WORKDIR respectively to the host machine
  3. Carry on with the steps of the original script

I tested the new script on my Mac and docker-compose started as expected.

Added support for Mac user for useing the command ./bin/docker-compose up -d
@MPP-SG MPP-SG changed the title Added support for Mac user fpr useing docker compose script script: Added support for Mac user for using docker compose script Sep 16, 2020
Copy link
Member

@otavio otavio left a comment

Choose a reason for hiding this comment

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

Outstanding! Thanks for looking at this!

I made two minor comments on the code. Please take a look.

Also, please add at the end of the commit log:

Fixes: #407 

So the issue is closed when we merge this.

bin/docker-compose Outdated Show resolved Hide resolved
bin/docker-compose Outdated Show resolved Hide resolved
Some prechecks at the beginning of the script are added

In the case statement if the OS is unknown (e.g. default case) then the script errors out

The if statements for setting the working directory are made easier to read

A couple of comments are written into the file to make it hopefully easier to understand for someone

Fixes: #407
@MPP-SG
Copy link
Author

MPP-SG commented Sep 23, 2020

@otavio Thank you for your feedback!
I hope it now is better to understand the code 😄

In the check summary I see, that one check failed due to an exceeding line length of 80 characters. Could you please point me in the right direction where I have to change something and how. Thank you 😄

updated Error Message for the pre checks
Copy link
Member

@otavio otavio left a comment

Choose a reason for hiding this comment

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

See the proposal I did. Please do whatever improvements you think are need.

Also, please squash the commits so we can merge it.

bin/docker-compose Outdated Show resolved Hide resolved
@MPP-SG
Copy link
Author

MPP-SG commented Sep 24, 2020

@otavio sorry for asking this again, but I don't get why the "Commit Message Check" is failing. My last commit message has just 19 characters...
Or does this check validating the line length in the file of the pull request? 🤔 But that would confuse me, because in the original file I think there were also lines with more than 80 characters... Or does this check only validate the line length of comments in the file?

@otavio
Copy link
Member

otavio commented Sep 24, 2020

@otavio sorry for asking this again, but I don't get why the "Commit Message Check" is failing. My last commit message has just 19 characters...

Yes; this was failing due to the pull request. I made a change to fix it so please rebase your patches and do a force push.

Also, the commits ought to be squashed into a single one. Please look at it as well, when rebasing.

gustavosbarreto added a commit that referenced this pull request Sep 30, 2020
gustavosbarreto added a commit that referenced this pull request Sep 30, 2020
This commit replaces PR #409. Closes #407
gustavosbarreto added a commit that referenced this pull request Sep 30, 2020
This commit replaces PR #409. Closes #407
otavio pushed a commit that referenced this pull request Sep 30, 2020
This commit replaces PR #409. Closes #407
@otavio
Copy link
Member

otavio commented Sep 30, 2020

Fixed by #464; closing this now.

@otavio otavio closed this Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants