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

fix xpi private cot verification #455

Merged
merged 4 commits into from
May 26, 2020

Conversation

escapewindow
Copy link
Contributor

Previously, we assumed that we would list any and all private github repos in constants.py. However, with the xpi project, we'll get many private repos that we will want to verify without needing to land a change in scriptworker.

Let's allow for ssh://github.com source urls to imply a private repository.

As ridealongs, I added --verbose and --no-check-task to verify-cot. I also cleaned up some extraneous output at the end of verify_cot.

Also, the second patch gets us to stop creating ... directories while running test_production.py tests.

The base_*_dir and task_log_dir_template config entries were from a historic, unmerged PR. Let's set the actual config paths inside our tempdir. Bonus: we won't have weird bustage related to hardcoded directory paths if and when we run two concurrent sets of tests.

I tested this patchset against the tasks in https://firefox-ci-tc.services.mozilla.com/tasks/groups/NFYpj38mQdSJGRxqG44_Iw , after setting SCRIPTWORKER_GITHUB_OAUTH_TOKEN to a token with read access to the private repo. I can invite you to my test-xpi-private repo if you want to give it a spin.

@Callek , @JohanLorenzo do you have cycles to look at this?

Previously, we assumed that we would list any and all private github
repos in constants.py. However, with the xpi project, we'll get many
private repos that we will want to verify without needing to land a
change in scriptworker.

Let's allow for ssh://github.com source urls to imply a private
repository.

As ridealongs, I added `--verbose` and `--no-check-task` to
`verify-cot`. I also cleaned up some extraneous output at the end of
`verify_cot`.
The base_*_dir and task_log_dir_template config entries were from a
historic, unmerged PR. Let's set the actual config paths inside our tempdir.
Bonus: we won't have weird bustage related to hardcoded directory paths if and
when we run two concurrent sets of tests.
Copy link
Contributor

@JohanLorenzo JohanLorenzo left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

# Avoid creating a `...` directory
for k,v in config.items():
if v == '...':
raise Exception(f"Let's not keep any '...' config values. {k} is {v}!")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

if (any(vcs_rule.get("require_secret") for vcs_rule in context.config["trusted_vcs_rules"])) and "github.com" in source_url:
if (
source_url.startswith("ssh://") or any(vcs_rule.get("require_secret") for vcs_rule in context.config["trusted_vcs_rules"])
) and "github.com" in source_url:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be ok if you wanted to drop require_secret entirely -- guardian no longer uses it... but its fine to leave.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. XPI doesn't have the requirement of releasing off these private source repos (we release off the manifest repo, which is public and known). We still have a check in is_pull_request that checks for has_commit_landed_on_repository. That check is essentially skipped if we have require_secret set. I don't know if we still need that behavior. I'll leave it for now, but we may want to revisit.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can't test has_commit_landed_on_repository for a private repository. I don't have a need for this skippage anymore for guardian at all, but its worth knowing that there is no other way to get this information [that I could find] which would let us use a token.

It's essentially github-web-secret-api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#456 for followup.
I may need actual private repo releases for adhoc, so hopefully I'll have a better understanding in a bit.

logging.basicConfig()
level = logging.DEBUG if opts.verbose else logging.INFO
log.setLevel(level)
logging.basicConfig(level=level)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@escapewindow escapewindow merged commit ffbed1e into mozilla-releng:master May 26, 2020
@escapewindow
Copy link
Contributor Author

Thank you both!

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