launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #27634
Re: [Merge] ~ilasc/launchpad:revision-status-submission-api into launchpad:master
There is a *lot* of formatting noise in this MP; for example it isn't at all clear what substantive changes have been made to `lib/lp/testing/factory.py` because they're buried under a pile of indentation changes and similar. I expect to see all of these unrelated changes reverted before I take another look at this, because it's difficult to review otherwise. It's probably a good idea to run `git diff master...` and look over the output (effectively doing a self-review) before you push anything.
Some of the rest of my comments may collide with work you've already done locally; if so, sorry about that, and hopefully you can make sense of them. I think these comments should at least be enough to let you move forward, although they aren't a complete review yet.
Diff comments:
> diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
> index 363356d..6c0db04 100644
> --- a/lib/lp/code/interfaces/gitrepository.py
> +++ b/lib/lp/code/interfaces/gitrepository.py
> @@ -1007,6 +1011,91 @@ class IGitRepositoryEdit(IWebhookTarget, IAccessTokenTarget):
> :raise: CannotDeleteGitRepository if the repository cannot be deleted.
> """
>
> + @operation_parameters(
> + name=TextLine(title=_("The name of the status report.")),
> + git_repository=Int(title=_("Reference to a GitRepository")),
The repository shouldn't need to be passed as a parameter, because this method is on `GitRepository` - the implementation could just use `self`.
> + commit_sha1=TextLine(title=_("The commit sha1 of the status report.")),
> + date_created=Datetime(
> + title=_("The time when this request was made")),
> + url=TextLine(title=_("The external link of the status report.")),
> + description=TextLine(title=_("The description of the status report.")),
> + result=List(
> + title=_("A list of report result statuses to filter by."),
> + value_type=Choice(vocabulary=RevisionStatusResult)),
> + date_started=Datetime(
> + title=_("The time when this report was started.")),
> + date_finished=Datetime(
> + title=_("The time when this report completed.")),
> + log=Int(title=_("Reference to a RevisionStatusArtifact")))
Integer IDs of database rows should almost never be used to refer to objects in the webservice. When passing references to objects, we use `Reference`, ensuring that they have appropriate canonical URLs.
I'm not sure that the log should be passed in this way, though. It might be better to design things so that the returned `RevisionStatusReport` has a method where the caller can attach a log, or else to pass it in as text here. Let's discuss this on Friday or thereabouts.
> + @scoped(AccessTokenScope.REPOSITORY_BUILD_STATUS.title)
> + @call_with(user=REQUEST_USER)
> + @export_write_operation()
> + @operation_for_version("devel")
> + def newRevisionStatusReport(user, name, git_repository, commit_sha1,
> + date_created, url=None, description=None,
> + result=None, date_started=None,
> + date_finished=None, log=None):
> + """Create a New Status Report and return its ID.
I know the spec says to return the ID here, but I'm dubious - it would be more usual to declare this as `@export_factory_operation()` rather than `@export_write_operation()` and just return the object, which will translate into a 201 response with the entry.
> +
> + :param name: The name of the new report.
> + :param description: The description of the new report.
> + :param commit_sha1: The commit sha1 for the report.
> + :param result: The result of the new report.
> + """
> +
> +
> +class IRevisionStatusReport(Interface):
This will probably need to be exported, have a suitable URL, etc.
> + id = Int(title=_("ID"), required=True, readonly=True)
> +
> + name = TextLine(
> + title=_("The name of the report."), required=True)
> +
> + git_repository = Reference(
> + title=_("The Git repository for which this report is built."),
> + # Really IGitRepository, patched in _schema_circular_imports.py.
> + schema=Interface, required=True, readonly=True)
> +
> + commit_sha1 = Reference(
> + title=_("The Git commit for which this report is built."),
> + schema=Interface, required=True, readonly=True)
> +
> + url = Attribute("The external url of the report.")
> +
> + description = Attribute("A short description of the report.")
> +
> + result = Choice(
> + title=_('Result of the report'), vocabulary=RevisionStatusResult)
> +
> + date_created = Datetime(
> + title=_("When the report was created."), required=True, readonly=True)
> + date_started = Datetime(
> + title=_("When the report was started."))
> + date_finished = Datetime(
> + title=_("When the report has finished."))
> + log = Reference(
> + title=_("The Git commit for which this report is built."),
> + schema=Interface)
This is supposed to be a reference to a `RevisionStatusArtifact`, not to a Git commit.
> +
> +
> +class IRevisionStatusReportSet(Interface):
> + """The set of all revision status reports."""
> +
> + def new(name, git_repository, commit_sha1, date_created=None,
> + url=None, description=None, result=None, date_started=None,
> + date_finished=None, log=None):
> + """Return a new revision status report.
> +
> + :param name: A text string.
> + :param git_repository: An `IGitRepository` for which the report
> + is being created.
> + :param commit_sha1: The sha1 of the commit for which the report
> + is being created.
> + :param date_created: The date when the report is being created.
> + """
> +
> + def findRevisionStatusReportById(id):
> + """Returns the RevisionStatusReport for a given Id."""
> +
>
> # XXX cjwatson 2015-01-19 bug=760849: "beta" is a lie to get WADL
> # generation working. Individual attributes must set their version to
> diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
> index 99d451b..51079c9 100644
> --- a/lib/lp/code/model/gitrepository.py
> +++ b/lib/lp/code/model/gitrepository.py
> @@ -292,6 +297,91 @@ def git_repository_modified(repository, event):
> send_git_repository_modified_notifications(repository, event)
>
>
> +@implementer(IRevisionStatusReport)
> +class RevisionStatusReport(StormBase):
> + __storm_table__ = 'RevisionStatusReport'
> +
> + id = Int(primary=True)
> +
> + name = Unicode(name='name', allow_none=False)
> +
> + git_repository_id = Int(name='git_repository', allow_none=False)
> + git_repository = Reference(git_repository_id, 'GitRepository.id')
> +
> + commit_sha1_id = Unicode(name='commit_sha1', allow_none=False)
> + commit_sha1 = Reference(commit_sha1_id, 'GitRef.commit_sha1')
> +
> + url = Unicode(name='url')
> +
> + description = TextLine(title=msg("The description of the status report."))
> +
> + result = EnumCol(
> + dbName='result', schema=RevisionStatusResult)
> +
> + date_created = DateTime(
> + name='date_created', tzinfo=pytz.UTC, allow_none=False)
> + date_started = DateTime(name='date_started', tzinfo=pytz.UTC)
> + date_finished = DateTime(name='date_finished', tzinfo=pytz.UTC)
> +
> + log = ForeignKey(
> + dbName='log', foreignKey='Revision')
This should be written in Storm style rather than SQLObject style:
log_id = Int(name='log', allow_none=True)
log = Reference(log_id, 'RevisionStatusArtifact.id')
> +
> + def __init__(self, name, git_repository, commit_sha1, date_created,
> + url=None, description=None, result=None, date_started=None,
> + date_finished=None, log=None):
> + super(RevisionStatusReport, self).__init__()
Note that in new code you can just use zero-argument `super()`, in Python 3 style.
> + self.name = name
> + self.git_repository = git_repository
> + self.commit_sha1 = commit_sha1
> + self.date_created = date_created
> + self.url = url
> + self.description = description
> + self.result = result
> + self.date_started = date_started
> + self.date_finished = date_finished
> + self.log = log
> +
> +
> +@implementer(IRevisionStatusReportSet)
> +class RevisionStatusReportSet:
> +
> + def new(self, name, git_repository, commit_sha1, date_created=None,
> + url=None, description=None, result=None, date_started=None,
> + date_finished=None, log=None):
> + """See `IGitRevisionStatusReportSet`."""
> + store = IStore(RevisionStatusReport)
> + date_created = UTC_NOW
> + report = RevisionStatusReport(name, git_repository, commit_sha1,
> + date_created=date_created, url=None,
> + description=None, result=None,
> + date_started=None, date_finished=None,
> + log=None)
Since you accept these as parameters, you should probably pass them through as well.
> + store.add(report)
> + return report
> +
> + def findRevisionStatusReportById(self, id):
> + return IStore(RevisionStatusReport).find(
> + RevisionStatusReport,
> + RevisionStatusReport.id == id)
> +
> +
> +class RevisionStatusArtifact(StormBase):
> + __storm_table__ = 'RevisionStatusArtifact'
> +
> + id = Int(primary=True)
> +
> + library_file_id = Int(name='library_file', allow_none=False)
> + library_file = Reference(library_file_id, 'LibraryFileAlias.id')
> +
> + report_id = Int(name='report', allow_none=False)
> + report = Reference(report_id, 'RevisionStatusReport.id')
> +
> + def __init__(self, library_file, report):
> + super(RevisionStatusArtifact, self).__init__()
> + self.library_file = library_file
> + self.report = report
> +
> +
> @implementer(IGitRepository, IHasOwner, IPrivacy, IInformationType)
> class GitRepository(StormBase, WebhookTargetMixin, AccessTokenTargetMixin,
> GitIdentityMixin):
> diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
> index a832554..61a7b3c 100644
> --- a/lib/lp/code/model/tests/test_gitrepository.py
> +++ b/lib/lp/code/model/tests/test_gitrepository.py
> @@ -572,6 +572,18 @@ class TestGitRepository(TestCaseWithFactory):
> self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
> self.assertEqual(7, recorder1.count)
>
> + def test_findRevisionStatusReport(self):
> + repository = removeSecurityProxy(self.factory.makeGitRepository())
> + name = self.factory.getUniqueUnicode('report-name')
> + commit_sha1 = hashlib.sha1(b"Some content").hexdigest()
> + report = self.factory.makeRevisionStatusReport(
> + name=name, git_repository=repository, commit_sha1=commit_sha1,
> + date_created=self.factory.getUniqueDate())
> +
> + results = getUtility(
> + IRevisionStatusReportSet).findRevisionStatusReportById(1)
This shouldn't hardcode the ID.
> + self.assertEqual([report], list(results))
> +
>
> class TestGitIdentityMixin(TestCaseWithFactory):
> """Test the defaults and identities provided by GitIdentityMixin."""
> @@ -4200,6 +4212,27 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
> self.assertEqual(
> InformationType.PUBLIC, repository_db.information_type)
>
> + def test_newRevisionStatusReport(self):
> + repository = self.factory.makeGitRepository()
> + requester = repository.owner
> + webservice = webservice_for_person(requester)
When you pass a person to `webservice_for_person`, it passes OAuth headers for that person. This doesn't make sense if you're going to send a personal access token. We'll probably want to come up with some appropriate helpers at some point, but for now, use `webservice_for_person(None)` instead.
> + with person_logged_in(requester):
> + repository_url = api_url(repository)
> +
> + secret, _ = self.factory.makeAccessToken(
> + owner=requester, target=repository,
> + scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS])
> + header = {'Authorization': 'Token %s' % secret}
> +
> + response = webservice.named_post(
> + repository_url, "newRevisionStatusReport",
> + headers=header, name="CI", git_repository=repository.id,
> + commit_sha1='823748ur9804376',
I would probably be inclined to use something that's a valid SHA-1 digest, not least because the database column is declared as `character(40)`. (Normally in tests I use something like `hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()`, or whatever.)
> + date_created=datetime.now(pytz.UTC).strftime("%Y%m%d-%H%M%S"),
> + description="120/120 tests passed")
You can't call `webservice.named_post` from inside a `person_logged_in` context; it calls into the Zope publisher in such a way that there mustn't be a login interaction open when it's called. Instead of this, get whatever attributes you need first (it looks like you just need to fetch the value of `repository.id`) and move this out an indentation level, like this: https://paste.ubuntu.com/p/4vsJrYM3rJ/
After that, you get a 405 Method Not Allowed error. This is because you've (rightly) declared the webservice export as `@operation_for_version("devel")`, but you haven't specified that API version at the client side. To fix this, pass the `default_api_version="devel"` parameter to `webservice_for_person`.
After that, you get a 400 error with `date_created: Value doesn't look like a date.` as the response body. To fix this, use `.isoformat()` rather than `.strftime(...)` to serialize the `datetime` object.
After *that*, you get a 500 error with a reasonably clear-looking traceback, so I hope you can take it from there.
> +
> + self.assertEqual(200, response.status)
> +
> def test_set_target(self):
> # The repository owner can move the repository to another target;
> # this redirects to the new location.
--
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/410373
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:revision-status-submission-api into launchpad:master.
References
