Skip to content
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

Ignore ghost user #53

Merged
merged 2 commits into from
Oct 31, 2020
Merged

Ignore ghost user #53

merged 2 commits into from
Oct 31, 2020

Conversation

miguelslemos
Copy link
Contributor

@miguelslemos miguelslemos commented Oct 27, 2020

@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #53 into master will increase coverage by 2.00%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   43.81%   45.82%   +2.00%     
==========================================
  Files          12       13       +1     
  Lines         372      371       -1     
==========================================
+ Hits          163      170       +7     
+ Misses        207      199       -8     
  Partials        2        2              
Impacted Files Coverage Δ
internal/check/valid_owner.go 10.78% <0.00%> (+0.87%) ⬆️
internal/check/valid_owner_check.go 87.50% <87.50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b6c2ef...8316766. Read the comment docs.

Copy link
Owner

@mszostok mszostok left a comment

Choose a reason for hiding this comment

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

Hi

many thanks for your contribution!

I have one suggestion. Instead of hard-coding the @ghost user name, it would be better to export the option which takes a list of owners e.g. users, teams etc. that should be ignored by Valid Owner Checker.

  1. Add IgnoreOwners []string `envconfig:"default=@ghost"` property in line 15 in file internal/check/valid_owner.go. In this way, this property can be automatically set by environment variable OWNER_CHECKER_IGNORE_OWNERS which can be populated with list of owners names that should be ignored and not validated e.g. "@user,@org/team1,@ghost"

  2. In constructor NewValidOwner handle new cfg property and provide a func shouldIgnoreGitHubOwner(name) which can be used in for _, ownerName := range entry.Owners loop in line 63:

			if v.shouldIgnoreGitHubOwner(ownerName) {
				continue
			}
  1. Update README.md that there is a new environment variable OWNER_CHECKER_IGNORE_OWNERS which can be used

  2. Update action.yml with new property

If you will any help then ping me :) or if you will not have time to finish that then I can take it over if you want 👍

}
}

func isValidUser(user string) bool {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
func isValidUser(user string) bool {
func isValidOwner(user string) bool {

owner is a generic name for users, teams and emails :)

@mszostok
Copy link
Owner

Hi, I decided to merge that PR as it is 100% valid. I extracted the generalization logic to a saparate issue 👍 The user facing behaviour will stay the same, so by default the @ghost user will be deleted :)

Once again, thanks for your contribution and increasing test coverage 🚀

FYI: Integration tests are not executed on forked PR but I've checked it locally and all green 👍

@mszostok mszostok merged commit 97fb795 into mszostok:master Oct 31, 2020
@mszostok mszostok linked an issue Oct 31, 2020 that may be closed by this pull request
@miguelslemos miguelslemos deleted the issue-52 branch March 8, 2021 19:55
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.

Discussion: Support to ignore @ghost
2 participants