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

Added GitHub Post-Receive URL feature. #84

Merged
merged 9 commits into from
Feb 19, 2012

Conversation

simensen
Copy link
Contributor

This should resolve both #81 and #67. Sorta related to #58.

  • User now has an apiToken field
  • apiToken is generated when the User is created
  • apiToken and GitHub Post-Receive URL is displayed on the user's profile page
  • There is a packagist:tokens:generate command to generate tokens

Once in place, it should look like this:

profile page with apiToken

Upgrade

php app/console doctrine:schema:update
php app/console packagist:tokens:generate

Currently, packagist:tokens:generate is pretty basic and just regenerates tokens for users who are currently missing a token. This is useful for upgrading to support tokens (just needs to be run once). Eventually this command can be updated to have a few options:

  • --missing generate tokens for any user missing a token
  • --for-user regenerate a token for a specific user

TODO

I had to copy a lot of functionality from UpdatePackagesCommand. This should eventually (sooner rather than later?) be refactored and extracted into its own class so that it can be better reused and tested.

The shared functionality has been moved to Package\Updater.

use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\HttpKernel\KernelInterface;
use Symfony\Component\Finder\Finder;
Copy link
Contributor

Choose a reason for hiding this comment

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

please clean all unused statements

@simensen
Copy link
Contributor Author

Big refactoring. Moved code to Package\Updater. We can rename that/move it if that doesn't make sense.


//
// We found the package that was referenced.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

you should remove the 2 empty comment lines

@simensen
Copy link
Contributor Author

So, we should get @Seldaek feedback on a few things that I don't have answers for:

  • Do the versions actually need to be sorted? If so, why? It isn't clear in the code how this matters.
  • Was there any particular reason that flush() was being called after the updateInformation() call?

use Packagist\WebBundle\Repository\Repository\RepositoryInterface;
use Composer\Package\Version\VersionParser;
use Composer\Repository\VcsRepository;
use Composer\Package\PackageInterface;
use Composer\Repository\RepositoryManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

is the RepositoryManager still used ?

public function __construct(RegistryInterface $doctrine, \DateTime $start = null)
{
$this->doctrine = $doctrine;
$this->start = null !== $start ? $start : new \DateTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the start date should be a constructor argument. It is not more generate than the $clearExistingVersions param which is specific to the update call here

@simensen
Copy link
Contributor Author

Updated per @stof recent comments.

@stof
Copy link
Contributor

stof commented Feb 17, 2012

you will need to update the PR once #93 is merged as the bug fix concerns the code moved to the updater in your PR.

return new Response(json_encode(array('status' => 'error', 'message' => 'Invalid credentials',)), 403);
}

if (! preg_match('~(github.com/[\w_\-\.]+/[\w_\-\.]+)$~', $payload['repository']['url'], $matches)) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a comment (I'll fix while merging), dots don't have to be escaped in character classes, and hyphens either - as long as they're the last or first char of the char class.

Copy link
Member

Choose a reason for hiding this comment

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

And also _ is part of \w

@Seldaek Seldaek merged commit c267224 into composer:master Feb 19, 2012
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.

3 participants