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
Add Github Actions Secrets to organization #2006
Add Github Actions Secrets to organization #2006
Conversation
@s-t-e-v-e-n-k Could you please check it? |
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 looking excellent, I just have one issue before merging.
github/Organization.py
Outdated
secret_name, | ||
unencrypted_value, | ||
visibility="all", | ||
selected_repository_ids=[], |
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.
It feels more true to our interpretation of the API to call this selected_repositories
and then allow users to pass either Repository
objects or ids, and then transform them to a list of IDs when we make the PUT request.
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.
@s-t-e-v-e-n-k Thank you for review.
I made changes to pass list of repos, similar approach as in Organization.create_team. Please check it
ccf8233
to
e18a777
Compare
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.
Loving the direction, I just have a few more concerns.
github/Organization.py
Outdated
"encrypted_value": payload, | ||
"visibility": visibility, | ||
} | ||
if selected_repositories: |
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 isn't guarded like the assert check, which means that if you don't pass visibility and do pass selected_repositories something that isn't a list of Repository objects, then we throw an exception.
Do we need to be more careful here and assert that it's an empy list (or NotSet) if visibility isn't selected?
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.
yeah, it this was not covered, thanks
Made changes to handle cases:
visibility==selected and selected_repositories is NotSet
visibility!=selected and selected_repositories is not NotSet
@s-t-e-v-e-n-k Please check it
github/Organization.pyi
Outdated
secret_name: str, | ||
unencrypted_value: str, | ||
visibility: str, | ||
selected_repositories: List[Repository], |
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 think you need to mark the visibility parameter with it's default value. Also, what does type checking return if you call create_secret() with no selected_repositories?
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.
Marked and set NotSet as default value for selected_repositories
e18a777
to
0c02c21
Compare
Lint failed, fix that up and we can get this merged. |
0c02c21
to
71c5d2c
Compare
@s-t-e-v-e-n-k fixed, please check |
Type hints this time:
|
71c5d2c
to
424eed5
Compare
fixed, please trigger check again |
Add functionality to create and delete Secrets for Organizations.
Usage: