Bug 1529990 - Fix Mobile bustage #313

JohanLorenzo · 2019-02-22T18:50:05Z

And add more checks to detect PRs

scriptworker/cot/verify.py

coveralls · 2019-02-25T14:24:13Z

Coverage remained the same at 100.0% when pulling df97b06 on JohanLorenzo:more-pr-checks into dc088ba on mozilla-releng:master.

escapewindow · 2019-02-26T16:55:16Z

scriptworker/github.py

+
+    """
+    if isinstance(url, str):
+        return url.startswith('https://github.com/')


I'm not sure if we also need to support the [email protected] ssh urls. This should work if we don't. I'm fine with addressing this in a followup if we need to support ssh.

As of not, I'm not aware of the usage of [email protected] nor ssh:// within chain of trust. Let's add it whenever the need comes up, I believe

escapewindow · 2019-02-26T16:59:16Z

scriptworker/task.py

    to keep this.

    This checks for the following things::

        * ``task.extra.env.tasks_for`` == "github-pull-request"
        * ``task.payload.env.MOBILE_HEAD_REPOSITORY`` doesn't come from an official repo
        * ``task.metadata.source`` doesn't come from an official repo, either
+        * The last 2 items are landed on the official repo


This is a good check. A thought: we may want to look into restricting by branch as well (must have landed on official release branches on the official repo). I'm aware that may be difficult -- we may have to restrict it to the last N commits on a branch. If this is doable and wanted, this is probably a followup.

Agreed. This implementation paves the way to #273. We just need to parse the bits of HTML returned by the branch_commits endpoint and we can know what branch the commit lives on.

I'm not sure how we can check if it's part of the last N commits of a branch, yet. The real github APIv3 may have something already.

Ah, I thought it might be easier to detect the last N commits of a branch, rather than just knowing which branch(es) a commit is on. If the latter is easier, it's preferable.

Mmm, I thought there was a way to get the last N commits of a branch, but this doesn't seem to be exposed after all: https://developer.github.com/v3/repos/branches/

branch_commits is our best alternative.

escapewindow · 2019-02-26T17:02:26Z

scriptworker/task.py

    """
    return any((
-        is_try(task, context.config['source_env_prefix']),
-        is_pull_request(context, task),
+        # XXX Cron tasks don't usually define 'taskcluster-github' as their schedulerId as they


Once we use standalone taskgraph in our github repos, we can look at standardizing how the taskgraph generates cron tasks so we can detect it here.

JohanLorenzo · 2019-02-26T17:11:10Z

I briefly tested this PR against production:

Cron jobs:

mitchhentges · 2019-02-26T21:36:37Z

scriptworker/github.py

+    return official_repo_owner == repo_owner
+
+
+def _is_git_full_hash(commit_hash):


Nit: since the input of this function isn't always a commit hash (it may be a branch or a tag), perhaps this should be called "revision" :)

Good call. Fixed.

mitchhentges

💯 :)

mitchhentges · 2019-02-26T21:37:57Z

scriptworker/github.py

+        """
+        # Revision may be a tag name. `branch_commits` doesn't work on tags
+        if not _is_git_full_hash(commit_hash=revision):
+            revision = self.get_tag_hash(tag_name=revision)


Is it possible that in this case, revision isn't a tag_name but a branch?

At the moment, that's not a supported case. get_tag_hash() will raise an error because the tag doesn't exist. I'm not aware of a case where we want to support it. If that comes up, I'm fine adding this support.

mitchhentges · 2019-02-26T21:59:55Z

scriptworker/github.py

+
+        repo = self._github_repository.html_url
+
+        url = '/'.join([repo.rstrip('/'), 'branch_commits', revision])


I think I probably went on the same goose chase as you:

"The Github API probably has a way to look up if a commit is in my repository (not including PRs)"

"No, but maybe the Github API has a way to find out what branch(es) a commit is on?"

"Ugh, no, but this guy pointed out the semi-API/semi-HTML-y "https://github.com/:user/:repo/branch_commits/:commit"

It's crazy that that's the nicest way of doing this 😠 😛

Yeah, that's exactly what happened! 😅

mitchhentges · 2019-02-26T23:15:38Z

scriptworker/github.py

+
+def _check_github_url_is_supported(url):
+    if not is_github_url(url):
+        raise ValueError('"{}" is not a supported GitHub URL!'.format(url))


Nit that isn't relevant for this PR: I wonder if it would make sense to have earlier validation for things like this?
Currently, we don't need if checks to assert that our variables are strings/numbers/whatever deep within our functions because we perform earlier validation of the task, and that allows the functions that operate on the task to be so much easier to read (and write).

In the same way. it could be beneficial to have validation of github urls in advance? It could make logic cleaner, since we could leverage expectations with a data type (e.g.: "my function parameter is GithubUrl, so it better be a Github URL") rather than a function call ("what does _check_github_url_is_supported(...) do? Oh, it can throw an error, gotcha").

This is a potentially subjective note. Not that big of a concern :)

If, as you suggest, we weren't using strings everywhere (particularly if we could lean on type-checking), we could get away without these checks.

However, given that this is security sensitive code, I'd rather err on the side of having more sanity checks, rather than less.

FWIW, I'm not recommending removing these checks, but rather moving them to the entry-point of untrusted data and doing validation there.
To represent that the string has been validated as the specialized type (github url string), we could use a type at that point.

But yeah, I agree, I don't think that we should lessen our sanity checks :)

Agreed on keeping this check here, for now.

mitchhentges · 2019-02-26T23:17:41Z

scriptworker/task.py


    Args:
        context (scriptworker.context.Context): the scriptworker context.
        task (dict): the task definition to check.

    Returns:
-        bool: True if it's a pull-request
+        bool: True if it's a pull-request. False if it either comes from the official repos or if
+        the origin can't be determined. In fact, some valid scriptworker tasks don't


I think that this comment is cut off?

Oops. Fixed.

mitchhentges · 2019-02-26T23:19:07Z

scriptworker/task.py

+    )
+    for repo_url, revision, can_skip in urls_revisions_and_can_skip:
+        # XXX In the case of scriptworker tasks, neither the repo nor the revision is defined
+        if not repo_url and can_skip:


What happens if not repo_url and can_skip is False? We expect extract_github_repo_and_name(...) to throw an error?

That's right. repo_url won't be a valid github URL and then _check_github_url_is_supported() raises a ValueError

mitchhentges · 2019-02-26T23:20:03Z

scriptworker/task.py


+        github_repository = GitHubRepository(repo_owner, repo_name, context.config['github_oauth_token'])
+        conditions.append(not await github_repository.has_commit_landed_on_repository(context, revision))


JohanLorenzo · 2019-03-05T14:51:32Z

Chatted with @tomprince, yesterday on Slack: he's fine with me landing the patch.

escapewindow reviewed Feb 22, 2019

View reviewed changes

scriptworker/cot/verify.py Show resolved Hide resolved

JohanLorenzo marked this pull request as ready for review February 25, 2019 13:36

JohanLorenzo closed this Feb 25, 2019

JohanLorenzo reopened this Feb 25, 2019

JohanLorenzo changed the title ~~[WIP] Fix Mobile bustage~~ Bug 1529990 - Fix Mobile bustage Feb 25, 2019

JohanLorenzo force-pushed the more-pr-checks branch from 535df6e to 4fdb421 Compare February 26, 2019 16:36

JohanLorenzo requested review from escapewindow, tomprince and mitchhentges February 26, 2019 16:41

escapewindow approved these changes Feb 26, 2019

View reviewed changes

mitchhentges reviewed Feb 26, 2019

View reviewed changes

mitchhentges approved these changes Feb 26, 2019

View reviewed changes

JohanLorenzo added 2 commits March 5, 2019 15:08

Bug 1529990 - Fix Mobile bustage

949025b

21.0.0

df97b06

JohanLorenzo force-pushed the more-pr-checks branch from c492c70 to df97b06 Compare March 5, 2019 14:17

JohanLorenzo removed the request for review from tomprince March 5, 2019 14:51

JohanLorenzo merged commit 5876847 into mozilla-releng:master Mar 5, 2019

JohanLorenzo deleted the more-pr-checks branch March 5, 2019 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1529990 - Fix Mobile bustage #313

Bug 1529990 - Fix Mobile bustage #313

JohanLorenzo commented Feb 22, 2019

coveralls commented Feb 25, 2019 •

edited

Loading

escapewindow Feb 26, 2019

JohanLorenzo Feb 26, 2019

escapewindow Feb 26, 2019

JohanLorenzo Feb 26, 2019

escapewindow Feb 26, 2019

JohanLorenzo Feb 27, 2019

escapewindow Feb 26, 2019

JohanLorenzo commented Feb 26, 2019 •

edited

Loading

mitchhentges Feb 26, 2019

JohanLorenzo Feb 27, 2019

mitchhentges left a comment

mitchhentges Feb 26, 2019

JohanLorenzo Feb 27, 2019

mitchhentges Feb 26, 2019

JohanLorenzo Feb 27, 2019

mitchhentges Feb 26, 2019

tomprince Feb 26, 2019

mitchhentges Feb 27, 2019

JohanLorenzo Feb 27, 2019

mitchhentges Feb 26, 2019

JohanLorenzo Feb 27, 2019

mitchhentges Feb 26, 2019

JohanLorenzo Feb 27, 2019

mitchhentges Feb 26, 2019

JohanLorenzo commented Mar 5, 2019

		return official_repo_owner == repo_owner


		def _is_git_full_hash(commit_hash):


		repo = self._github_repository.html_url

		url = '/'.join([repo.rstrip('/'), 'branch_commits', revision])


		github_repository = GitHubRepository(repo_owner, repo_name, context.config['github_oauth_token'])
		conditions.append(not await github_repository.has_commit_landed_on_repository(context, revision))

Bug 1529990 - Fix Mobile bustage #313

Bug 1529990 - Fix Mobile bustage #313

Conversation

JohanLorenzo commented Feb 22, 2019

coveralls commented Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanLorenzo commented Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchhentges left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanLorenzo commented Mar 5, 2019

coveralls commented Feb 25, 2019 •

edited

Loading

JohanLorenzo commented Feb 26, 2019 •

edited

Loading