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

Enables GIT_REPOs with other protocols (http instead of https) #3

Merged
merged 2 commits into from Jul 7, 2017

Conversation

ehirsch
Copy link
Contributor

@ehirsch ehirsch commented Jul 5, 2017

I am talking about the git url which is used to load the static config. I changed the creation of said url in a way that you now have to option to define it with an optional prefixed protocol.
It is now possible to define GIT_REPO as:

The protocol is going to be respected or respectively added to the URL.

I also created a bash script for tests which is run whenever the unit tests are run. But I just learned that run-unit-tests.sh is deprecated, so you probably want to change the way the script is executed. I was unable to find the right place.

@DonMartin76
Copy link
Member

Oh shoot, I just realized you did the changes on master. Please branch off from next please, then I will merge as soon as you fixed the indentation (see review).

@ehirsch ehirsch changed the base branch from master to next July 7, 2017 11:27
@ehirsch
Copy link
Contributor Author

ehirsch commented Jul 7, 2017

I was only able to change the target branch to next. Is this enough? Where do I find the review?

echo "Assuming public repository, GIT_CREDENTIALS is empty"
GIT_URL=${protocol}://${repo}
fi

Copy link
Member

Choose a reason for hiding this comment

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

I really like this approach; may I just nitpick and ask you to make the indentation correct?

Copy link
Member

Choose a reason for hiding this comment

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

My bad, I didn't submit the review :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tabs or space?

Copy link
Member

Choose a reason for hiding this comment

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

Spaces, always spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

better?

@DonMartin76
Copy link
Member

LGTM

@DonMartin76 DonMartin76 merged commit 1df7ce6 into apim-haufe-io:next Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants