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

Adds a composer-lock badge #153

Merged
merged 4 commits into from
May 30, 2016

Conversation

heiglandreas
Copy link
Contributor

No description provided.

@liuggio
Copy link
Member

liuggio commented May 13, 2016

@heiglandreas this looks good I'll try in a day I'm in a conference right now 👍

@heiglandreas
Copy link
Contributor Author

No Problem at all! ;)

Take your time and say Hi to everyone at PHPDay ;)

{
$repo = str_replace('.git', '', $repository->getOriginalObject()->getRepository());

$client = new Client();
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to inject and set this into the constructor so we can mock up during test
what do you think? @heiglandreas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same actually but wanted to see what the test-results would show actually...I'll refactor that later today

Copy link
Member

Choose a reason for hiding this comment

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

Is possible to add short timeout like 2 secs on client service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. That reminds me that I should also add a fallback like "Couldn't test" in case no result could be fetched!

@heiglandreas
Copy link
Contributor Author

The test should have failed though....

$repo = str_replace('.git', '', $repository->getOriginalObject()->getRepository());

$client = new Client();
$response = $client->get($repo . '/blob/master/composer.lock');
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking do we need the GET or maybe is better the HEAD? would be faster ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HEAD would be OK. Get would allow to check whether the composer.lock actually is a composer lock at a later point in time. But then we can move from HEAD to GET as well.

Good spot!

This commit

* adds working tests,
* alters retrieval from GET to HEAD
* adds a timeout
* adds the GuzzleClient via DI
@liuggio
Copy link
Member

liuggio commented May 14, 2016

@heiglandreas great ! What about the frontend stuff?

This commit adds some frontend-stuff but the automatic change of the
composer.lock-badge when a different repository is given does not yet
work as somehow the composer.lock-information is not sent when calling
the snippets.
@heiglandreas
Copy link
Contributor Author

I can't seem to include the new composerlock information into the /snippet/all-route. I've added the infos to the config but it's like that's not read. Any hints on where I should take a look at?

Thanks

@heiglandreas
Copy link
Contributor Author

I don't believe it! In travis the information suddenly is available and causes the tests to fail. But I can't see it in the browser....

@heiglandreas
Copy link
Contributor Author

Fixed the Browser-issue on my end!

@heiglandreas
Copy link
Contributor Author

@liuggio Anything else I can/should do?

@liuggio
Copy link
Member

liuggio commented May 30, 2016

@heiglandreas great today I'll deploy it :)

@liuggio liuggio merged commit aa54f2f into PUGX:master May 30, 2016
@liuggio
Copy link
Member

liuggio commented May 30, 2016

@heiglandreas sorry ... did you already tested for non-git repository?

@liuggio
Copy link
Member

liuggio commented May 30, 2016

@heiglandreas sorry I had to revert the deploy caused by a problem on prod with dependencies with redisbundle snc/SncRedisBundle#172 (comment) but I've resolved ... I'll try to do it tonight or tomorrow 👍

@heiglandreas
Copy link
Contributor Author

That gives me the time to test this with non-git repos.... Do you have an example for a non-git repo? You mean like svn? I'll have to dig through that then.

@liuggio
Copy link
Member

liuggio commented May 31, 2016

@heiglandreas we are online with your code 💃
:) http://screencloud.net/v/g8Va

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.

2 participants