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

Fix lint only changes, full lint on merge forwards #50583

Merged
merged 1 commit into from Nov 27, 2018

Conversation

Projects
None yet
3 participants
@damon-atkins
Copy link
Member

commented Nov 20, 2018

What does this PR do?

  • lint only changes, previous diff picked up out of data files, when the branch was out of date.
  • full lint on merge forward to pick up changes in the lint checks between versions.

What issues does this PR fix or reference?

  • Errors with pylint itself were not picked up, resulting in a pass.

Previous Behavior

  • When a branch was out of date, more files were linted than required.

New Behavior

  • Lint only the files changes in the PR
  • If the lint of "change files only" passed, Lint all files as well if a merger forward i.e. branch name starts with merge-

@cachedout cachedout requested a review from dubb-b Nov 20, 2018

Show resolved Hide resolved .ci/lint
Show resolved Hide resolved .ci/lint

@damon-atkins damon-atkins force-pushed the damon-atkins:jenkins_pylint branch from 16eea72 to 1c88fbf Nov 21, 2018

Show resolved Hide resolved .ci/lint Outdated
Fix lint only changes, full lint on merge forwards
- lint only changes previous diff picked up out of data files, when the branch was out of date.
- full limit on merge forward to pick up changes in the lint checks between versions.
- update CODEOWNERS for .ci/*

@damon-atkins damon-atkins force-pushed the damon-atkins:jenkins_pylint branch from 8ef3a23 to 2d1f51c Nov 22, 2018

@dubb-b

This comment has been minimized.

Copy link

commented Nov 24, 2018

@damon-atkins after talking to @cachedout and a few others we may have decided that we only want to do the changes and not the full lint on a merge forward.

@cachedout if you change your mind please let me know. The full lint would only run on a merge forward PR and only if the changes passes. It may be handy to have it run only in that case. I'd like your or @saltstack/team-core 's input here.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2018

lint has coding rules, which change from one release to the next. The idea is to try and make sure those changes are picked up. Hence it only happens if the branch starts with merge- and only if the change files pass also. i.e for a merge-* branch, lets check the changes and if passed make extra sure the rest of the code is ok as well, for any other branch name else just check the changes.

Happy to pull the code. Just thought it might help the quality of the releases.

The original code which listed out the changes detected "files out of date" when a branch is old. This is fixed in this PR after running some experiments to determine the best git diff args to use.

One choice is to try it out and pull code if it becomes an issue to the merge forward process.

I have assume that a full lint will take less time than all the other tests what are run, so their should be no delays.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Nov 26, 2018

I am fine with trying this out for a while if @dubb-b is.

@dubb-b

This comment has been minimized.

Copy link

commented Nov 26, 2018

I am fine with this.

Proceed with merge if no one has objections.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

Hi @dubb-b can you approve the PR please.

@dubb-b

dubb-b approved these changes Nov 27, 2018

@cachedout cachedout merged commit 6317f3a into saltstack:2017.7 Nov 27, 2018

10 checks passed

WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details

@damon-atkins damon-atkins deleted the damon-atkins:jenkins_pylint branch Nov 28, 2018

cachedout pushed a commit that referenced this pull request Nov 29, 2018

Mike Place
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.