Skip to content

Support creation of Dependabot Organization and Repository Secrets#2874

Merged
EnricoMi merged 25 commits intoPyGithub:mainfrom
thomascrowley:main
Mar 21, 2024
Merged

Support creation of Dependabot Organization and Repository Secrets#2874
EnricoMi merged 25 commits intoPyGithub:mainfrom
thomascrowley:main

Conversation

@thomascrowley
Copy link
Copy Markdown
Contributor

@thomascrowley thomascrowley commented Jan 17, 2024

Inline with #2284, this change allows the creation and interactions with both "actions" and "dependabot" secrets both on the organization or a repository.

To achieve this I have added an additional parameter to all related methods to allow you to choose between "actions" or "dependabot" secrets. The default value is set to "actions" to help with back compatibility.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 18, 2024

Codecov Report

Attention: Patch coverage is 92.00000% with 2 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@9e09245). Click here to learn what that means.

❗ Current head c72f22b differs from pull request most recent head 8ad1bf7. Consider uploading reports for the commit 8ad1bf7 to get more accurate results

Files Patch % Lines
github/Repository.py 85.71% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2874   +/-   ##
=======================================
  Coverage        ?   96.73%           
=======================================
  Files           ?      147           
  Lines           ?    14895           
  Branches        ?        0           
=======================================
  Hits            ?    14408           
  Misses          ?      487           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomascrowley thomascrowley marked this pull request as ready for review January 18, 2024 13:36
@thomascrowley thomascrowley changed the title Enable the creation of Dependabot Organization Secrets Enable the creation of Dependabot Organization and Repository Secrets Jan 18, 2024
@thomascrowley thomascrowley changed the title Enable the creation of Dependabot Organization and Repository Secrets feat: Enable the creation of Dependabot Organization and Repository Secrets Jan 18, 2024
@EnricoMi EnricoMi changed the title feat: Enable the creation of Dependabot Organization and Repository Secrets Support creation of Dependabot Organization and Repository Secrets Jan 22, 2024
@thomascrowley
Copy link
Copy Markdown
Contributor Author

Hi @EnricoMi please can you re-review. I believe we've now addressed all your comments.

Comment on lines +589 to +594
def testOrgGetSecretAssertion(self):
self.org = self.g.get_organization("demoorg")
try:
self.org.get_secret(secret_name="splat", secret_type="supersecret")
except AssertionError:
assert True
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What exactly are you testing here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test confirms that if you pass in a secret_type that isn't "dependabot" or "actions" it successfully throws an error.

I added this because codecov flagged this as untested

Comment on lines +1973 to +1978
def testRepoGetSecretAssertion(self):
repo = self.g.get_repo("demoorg/demo-repo-1")
try:
repo.get_secret(secret_name="splat", secret_type="supersecret")
except AssertionError:
assert True
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here, what does this test assert?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test confirms that if you pass in a secret_type that isn't "dependabot" or "actions" it successfully throws an error.

I added this because codecov flagged this as untested

@thomascrowley
Copy link
Copy Markdown
Contributor Author

hi @EnricoMi is this ok to be merged now?

smkillen and others added 4 commits February 27, 2024 11:53
* Moving tests to respective files

* Correcting typo

* Correcting typo

* Creating ReplayData and Fixing tests, updating Organization.py to take secret_type on secrets call
Co-authored-by: Enrico Minack <github@enrico.minack.dev>
Co-authored-by: Enrico Minack <github@enrico.minack.dev>
@thomascrowley
Copy link
Copy Markdown
Contributor Author

Hello @EnricoMi I believe we've addressed all comments, can this be merged in?

Copy link
Copy Markdown
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

Can you adopt this for all exception assertions, please?

@jesdi
Copy link
Copy Markdown

jesdi commented Mar 4, 2024

Hi guys, thanks for working on this, we also would benefit a lot from adding a better support for Dependabot's secrets. 🙏

@jesdi
Copy link
Copy Markdown

jesdi commented Mar 4, 2024

I think there is missing also the delete_secret to support also dependabot secrets.
You can cherry pick the commit from my fork @thomascrowley jesdi@a1ef679
Thanks again!

smkillen and others added 4 commits March 13, 2024 15:48
* Review Comments: Changing Testing of secret_type assertion

* update delete secret option

---------

Co-authored-by: Thomas Crowley <15927917+thomascrowley@users.noreply.github.com>
@thomascrowley thomascrowley requested a review from EnricoMi March 20, 2024 14:21
Copy link
Copy Markdown
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

LGTM!

@EnricoMi EnricoMi merged commit 0784f83 into PyGithub:main Mar 21, 2024
@thomascrowley
Copy link
Copy Markdown
Contributor Author

LGTM!

Thanks for the approval @EnricoMi

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.

5 participants