-
-
Notifications
You must be signed in to change notification settings - Fork 336
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
Opt-in resources request mechanism #767
Conversation
This PR adds the ability to request grant/revoke access to an Opt-in ci resource (cirun in this case). This depends on conda-forge/conda-smithy#1703, which adds this functionality in the Requesting or Revoking CI Resources AccessFor specific CI resources that are available on an opt-in basis, you can request access by submitting a PR with your feedstock name added to a relevant file under the List of opt-in resources:
more of these will be added later, this is to get us started. We're using txt files, for this via PR mechanism for the following reasons:
|
import requests | ||
from ruamel.yaml import YAML | ||
|
||
GH_ORG = os.environ.get("GH_ORG", "conda-forge") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we are offering admin-requests
outside conda-forge so we could hardcode this, but this doesn't hurt either 👌
access_control.py
Outdated
|
||
GH_ORG = os.environ.get("GH_ORG", "conda-forge") | ||
|
||
CIRUN_FILENAME_RESOURCE_MAPPING = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the configuration dictionary that maps {grant,revoke}_access/<name>.txt
to an actual resource in ... .access_control.yml
? Or where? Can you document the expected schema with a #: comment
on top?
@jaimergp, @aktech, this is good to go right? (We need conda-forge/conda-smithy#1793 to clean this PR a little bit and conda-forge/conda-smithy#1794 to handle the teams correctly, but it should be possible to test without them) |
I think so. There might be typos, mismatching identifiers, or simple errors that we will easily reveal once we try this live. The docs (as per comment above) seem to be outdated too, but I suggest we amend them once we know it's working. One possible pain point is the maintenance of the access_control YAML here (for bookkeeping), because it might become a conflict source across PRs, so we need to ensure it's written in a consistent way (sort keys, etc). Again the kind of thing we will easily observe live. |
I ran this on a different branch and fixed a bunch of failures. |
This is ready for a review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for the review. I'll start some docs PRs. |
Awesome! Thanks! Please ping me in the docs PR when needed! |
WIP. See #767 (comment)
Closes #752.