- 
                Notifications
    
You must be signed in to change notification settings  - Fork 5
 
          chore(ci): goheader linting only runs in CI on changed or new files
          #100
        
          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
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.
should the CI check only apply to modified files? Seems odd to have to update files just for this.
          
 I agree. We should use this option.  | 
    
399d5e6    to
    f61df6a      
    Compare
  
    | 
           Ideally we should run only  In the meantime, let's set  PS: side thing I found, added in #88: Use   | 
    
2a70c80    to
    6c43849      
    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.
Summary of changes, per existing discussion plus separately with legal:
- Only flag changed files
 -  Change the template to require the first year the file was modified and, if not this year, a range to now. I think I tried to do that in the beginning but 
MOD-YEAR-RANGEdoesn't seem to play nice when it's not actually a range. 
d33c6ae    to
    4ddf49f      
    Compare
  
    
          
 It works fine for me, it fails with error  Obviously the perfect solution would be a linter handling the git plumbing to find only changed files and scan headers for only those files, one can only dream 🤷 Unless it exists let me know!  | 
    
4ddf49f    to
    2833156      
    Compare
  
    goheader linting only runs in CI on changed or new files
      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.
Approving with minor changes requested.
| with: | ||
| version: v1.60 | ||
| only-new-issues: true | ||
| args: --enable goheader | 
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.
Please add a comment explaining why this is "manually" enabled via flag instead of in the config.
| - gocheckcompilerdirectives | ||
| - gofmt | ||
| - goheader | ||
| - goimports | 
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.
Please add either:
- A comment where 
goheaderwould have been, pointing readers to the workflow config; or - Copy the comment from the workflow config to here (with modification if necessary).
 
(1) feels better but I'm ok with the redundancy of 2 because this isn't depended on by any code / requires devs to read it regularly.
| 
           @ARR4N Whoops I think that got auto-merged on your approval, I did #101 with your suggestions 👍 PS: we should be careful about approving & auto-merge enabled.  | 
    
Why this should be merged
Unblock the CI and update the copyright
How this works
Bumped the year
How this was tested
Lint job passing