Opt out private/public repos by optOutPrivateRepos/optOutPublicRepos #54
Conversation
five510
force-pushed
the
feat/access-repos-config
branch
from
84c2072
to
fe79d00
Compare
|
Thank you for looking into this! I think it would be good for the config to be a part of As mentioned, all the policies include An example of getting the repo info is here: https://github.com/ossf/allstar/blob/main/pkg/policies/branch/branch.go#L158-L161 |
five510
force-pushed
the
feat/access-repos-config
branch
from
ebb4eb9
to
2ba4f4a
Compare
five510
changed the title
|
@jeffmendoza |
|
Sounds good, I'll take a look. |
opt-out.md
Outdated
|
|
||
| optConfig: | ||
| optOutStrategy: true | ||
| optOutRepos: | ||
| - my-repo-name-here | ||
|
|
||
| To opt-out all private/public repositories, submit a PR to that `.allstar` repo, and add `optOutPrivateRepos` or `optOutPublicRepos`. ex: |
There was a problem hiding this comment.
I'd like to keep this file focused on a single repository opt-out. Please revert this, and add this section to the end of the section in https://github.com/ossf/allstar/blob/main/README.md#enable-configuration
pkg/config/config.go
Outdated
| GetContents(context.Context, string, string, string, | ||
| *github.RepositoryContentGetOptions) (*github.RepositoryContent, | ||
| []*github.RepositoryContent, *github.Response, error) | ||
| } | ||
|
|
||
| // IsEnabled determines if a repo is enabled by interpreting the provided | ||
| // org-level and repo-level OptConfigs. | ||
| func IsEnabled(o OrgOptConfig, r RepoOptConfig, repo string) bool { | ||
| func IsEnabled(ctx context.Context, o OrgOptConfig, r RepoOptConfig, rep repositories, owner, repo string) (bool, error) { |
There was a problem hiding this comment.
It's not good to have a public function with a parameter of a private type. Also, all the functions/packages only pass around github.Client in code currently. Make IsEnabled() take a github.Client then simply call a private isEnabled() which takes a repositories, then change each policy to pass in the client, not client.Repositories. The tests should then call the private isEnabled with the repositories mock object.
There was a problem hiding this comment.
It makes sense. Updated with a57b1eb.
pkg/config/config_test.go
Outdated
| *github.Response, error) { | ||
| b := test.Org.OptOutPrivateRepos | ||
| return &github.Repository{ | ||
| Private: &b, |
There was a problem hiding this comment.
This should be a separate input for each test, the mock response from github. Then the org config as you have it, then the expected output.
There was a problem hiding this comment.
Thanks. Added mock config with d86295d.
| enabled, err := config.IsEnabled(ctx, oc.OptConfig, rc.OptConfig, c.Repositories, owner, repo) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Looks like config.IsEnabled will need to be mocked now for all policies, similar to how configFetchConfig is currently mocked. It probably should have been mocked to begin with, as the policy tests don't need to unit test IsEnabled. The policy tests should now have the return value of IsEnabled be an input instead of the org/repo optconfigs.
There was a problem hiding this comment.
Thanks. Updated with c59738b.
|
@jeffmendoza Could you please check it again? Thanks! |
|
@five510 This deployed yesterday and looks like it is working great. Thanks for your contribution!! |
Changes