Differential D25011 Diff 33 src/applications/differential/storage/DifferentialDiff.php

Changeset View

Standalone View

src/applications/differential/storage/DifferentialDiff.php

<?php		<?php

final class DifferentialDiff		final class DifferentialDiff
extends DifferentialDAO		extends DifferentialDAO
implements		implements
PhabricatorPolicyInterface,		PhabricatorPolicyInterface,
PhabricatorExtendedPolicyInterface,		PhabricatorExtendedPolicyInterface,
HarbormasterBuildableInterface,		HarbormasterExternalBuildableInterface,
HarbormasterCircleCIBuildableInterface,
HarbormasterBuildkiteBuildableInterface,		HarbormasterBuildkiteBuildableInterface,
PhabricatorApplicationTransactionInterface,		PhabricatorApplicationTransactionInterface,
PhabricatorDestructibleInterface,		PhabricatorDestructibleInterface,
PhabricatorConduitResultInterface {		PhabricatorConduitResultInterface {

protected $revisionID;		protected $revisionID;
protected $authorPHID;		protected $authorPHID;
protected $repositoryPHID;		protected $repositoryPHID;
▲ Show 20 Lines • Show All 542 Lines • ▼ Show 20 Lines	return array(
pht('The ref name for this change in the staging repository.'),		pht('The ref name for this change in the staging repository.'),
);		);
}		}

public function newBuildableEngine() {		public function newBuildableEngine() {
return new DifferentialBuildableEngine();		return new DifferentialBuildableEngine();
}		}

		public function getExternalBuildableRef() {
		$ref = $this->getStagingRef();
		$ref = preg_replace('(^refs/tags/)', '', $ref);
		speckUnsubmitted Not Done Inline Actions Should any of this be abstracted by the VCS in use? I'm in the minority with using mercurial but I would eventually like to build out better mercurial support. speck: Should any of this be abstracted by the VCS in use? I'm in the minority with using mercurial…
		deadalnixAuthorUnsubmitted Done Inline Actions Maybe eventually. Right now it isn't, and CircleCI require th us of github, either as staging or as a main repo tracked by phabricator, so either way, it must be git. deadalnix: Maybe eventually. Right now it isn't, and CircleCI require th us of github, either as staging…
		speckUnsubmitted Not Done Inline Actions That makes sense. speck: That makes sense.
		return array(
		HarbormasterExternalBuildableInterface::TAG,
		$ref,
		);
		}

/* -( HarbormasterCircleCIBuildableInterface )----------------------------- */		public function getRepository() {
		speckUnsubmitted Not Done Inline Actions Since this issues a query this might be better named as `queryRepository()` or `loadRepository()` speck: Since this issues a query this might be better named as `queryRepository()` or `loadRepository…
		deadalnixAuthorUnsubmitted Done Inline Actions That's fine by me. deadalnix: That's fine by me.
		deadalnixAuthorUnsubmitted Done Inline Actions So actually, I remember why I went with that (going back into th code to change it did help). `PhabricatorRepositoryCommit` already has a `getRepository` method that does exactly that and need to implement this interface, so I went with that rather than change `PhabricatorRepositoryCommit`. deadalnix: So actually, I remember why I went with that (going back into th code to change it did help).
		speckUnsubmitted Not Done Inline Actions Oh okay - it's hard to see that this overrides an existing function. I'm used to Java which requires `@Override` annotations on methods which are overriding implementations. speck: Oh okay - it's hard to see that this overrides an existing function. I'm used to Java which…


public function getCircleCIGitHubRepositoryURI() {
$diff_phid = $this->getPHID();
$repository_phid = $this->getRepositoryPHID();		$repository_phid = $this->getRepositoryPHID();
if (!$repository_phid) {		if (!$repository_phid) {
throw new Exception(		return null;
pht(
'This diff ("%s") is not associated with a repository. A diff '.
'must belong to a tracked repository to be built by CircleCI.',
$diff_phid));
}		}
speckUnsubmitted Not Done Inline Actions It might make sense to keep this as part of the implementation of `getRepository()`. I guess it depends how likely this is to be called from multiple places which would have to do similar error handling. speck: It might make sense to keep this as part of the implementation of `getRepository()`. I guess it…
deadalnixAuthorUnsubmitted Done Inline Actions I removed it from there because the error message is specific to CircleCI and that really doesn't belong here. Instead, it returns null and the CircleCI handler generate an error. deadalnix: I removed it from there because the error message is specific to CircleCI and that really…
speckUnsubmitted Not Done Inline Actions The error message could maybe be generalized - I'm not familiar enough with what this functionality is expecting to do. The only thing I would say to consider is how the `PhabricatorRepositoryQuery->executeOne()` error behaves (does it return null or throw up?). In either case I think we should add comment to the method to clarify the error behavior. speck: The error message could maybe be generalized - I'm not familiar enough with what this…

$repository = id(new PhabricatorRepositoryQuery())		return id(new PhabricatorRepositoryQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())		->setViewer(PhabricatorUser::getOmnipotentUser())
->withPHIDs(array($repository_phid))		->withPHIDs(array($repository_phid))
->executeOne();		->executeOne();
if (!$repository) {
throw new Exception(
pht(
'This diff ("%s") is associated with a repository ("%s") which '.
'could not be loaded.',
$diff_phid,
$repository_phid));
}

$staging_uri = $repository->getStagingURI();
if (!$staging_uri) {
throw new Exception(
pht(
'This diff ("%s") is associated with a repository ("%s") that '.
'does not have a Staging Area configured. You must configure a '.
'Staging Area to use CircleCI integration.',
$diff_phid,
$repository_phid));
}

$path = HarbormasterCircleCIBuildStepImplementation::getGitHubPath(
$staging_uri);
if (!$path) {
throw new Exception(
pht(
'This diff ("%s") is associated with a repository ("%s") that '.
'does not have a Staging Area ("%s") that is hosted on GitHub. '.
'CircleCI can only build from GitHub, so the Staging Area for '.
'the repository must be hosted there.',
$diff_phid,
$repository_phid,
$staging_uri));
}

return $staging_uri;
}

public function getCircleCIBuildIdentifierType() {
return 'tag';
}

public function getCircleCIBuildIdentifier() {
$ref = $this->getStagingRef();
$ref = preg_replace('(^refs/tags/)', '', $ref);
return $ref;
}		}


/* -( HarbormasterBuildkiteBuildableInterface )---------------------------- */		/* -( HarbormasterBuildkiteBuildableInterface )---------------------------- */

public function getBuildkiteBranch() {		public function getBuildkiteBranch() {
$ref = $this->getStagingRef();		$ref = $this->getStagingRef();

▲ Show 20 Lines • Show All 192 Lines • Show Last 20 Lines