-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: support private repository configuration #265
feat: support private repository configuration #265
Conversation
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.
Couple of nits and still looking at rest of PR. Still determining if switch to ruamel.yaml
from PyYAML
is the best choice. It looks like it but confirming.
Small suggestion, when making a change like that, maybe link to something like this in your PR description to show comparison. wdyt?
@jmeridth I've added the needed
@jmeridth I've added the needed changes and also mentioned the differences between
|
Thank you. Yeah, the library is built on PyYAML and has many improvements. I'm currently leaning towards a merge but double checking a few things. I do agree this new library simplifies multiple things we were handling manually (i.e., indention). |
password: "${{secrets.password}}" | ||
``` | ||
|
||
The principal key of each configuration need to match the package managers that the [script is looking for](https://github.com/github/evergreen/blob/main/dependabot_file.py#L78). |
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.
Can we change this link to a specific SHA(permalink)? If we add any changes to that file line 78 won't be the same.
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 agree @jmeridth , for example now it's already moved to another place. Should we add this page instead from the oficial documentation?
https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#package-ecosystem
Even though currently the script does not support all the available packages I believe the missing ones shouldn't be an issue to add.
Checking the list the supported package-ecosystems
still not added to the script are:
- devcontainers
- elm
- gitsubmodule
- gradle
- pub
- swift
Following the same logic that is now implemented it shouldn't be too much dificult to add new supported packages.
Thanks
if dependabot_file is None: | ||
print("\tNo (new) compatible package manager found") | ||
continue |
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.
@jmeridth found this issue that I've mistakenly introduced by removing the continue if a repository does not need any dependabot file the code breaks.
This continue should be here to break the for loop and continue to other repositories. Could this be added to the existing release or if necessary I can create a small fix PR and also add the changes mentioned here
Pull Request
Proposed Changes
This PR intends to allow the configuration of private registries on the
dependabot.yml
file based on a file created on the repository.Should fix #199
The following changes were done to the code:
ruamel.yaml
library it should be easier to add more configurations to the yaml if needed in the future. Differences to PyYamlREADME
file with the usage and some examplesExample: (Testers needed if possible)
Setting the environment variable as follow:
It expects a file with the name
dependabot-config.yaml
to exist on the repository and it should have the following structure to add the needed private repositories:The action code will check if there is any key on the file that matches the ones that dependabot is looking for and will add the following to the configuration of each
package-ecosystem
configuration based on the found package:Current code coverage --> Still missing some tests regarding the existence of a configuration file (The existing tests were adapted to the current configuration)
Readiness Checklist
Author/Contributor
make lint
and fix any issues that you have introducedmake test
and ensure you have test coverage for the lines you are introducing@jeffrey-luszcz
Reviewer
fix
,documentation
,enhancement
,infrastructure
,maintenance
orbreaking