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

Jenkins CI Lint Improvements #50026

Merged
merged 3 commits into from Oct 22, 2018

Conversation

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

commented Oct 13, 2018

What does this PR do?

Improvements in .ci/lint

  • Change the output extension from .xml to .log (plain text) so Jenkins sends the correct mime type to browsers.
  • Remove the colour escape codes from the logs, so Artifact does not contain them, but the console output record by Jenkins does.
  • Store an Artifact of which files have changed, and also the files which have been deleted, for debugging/verifying the files that have been reviewed by pylint.
    *Remove the need to use find to scan the file system and call git multiple times. (small decrease in overhead)
    *Only run a stage if required.

What issues does this PR fix or reference?

Some general improvements.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

Best I could do is a syntax check to check if it will work.

Testing which needs to be performed:

  • Not sure how readFile() handles line feeds, have left out \\n or $ in the regex due to this.
  • General testing like create/alter a python file.

Another option is consider, is testing if people have submitted new code with .PY or .pY or .Py extension, as the original code would skip these files. (not that many people would use anything other than .py)

@damon-atkins damon-atkins changed the title [WIP] CI Improvement CI Lint Improvements Oct 13, 2018

@gtmanfred gtmanfred requested a review from KaiSforza Oct 14, 2018

@cachedout cachedout requested a review from dubb-b Oct 16, 2018

.ci/lint Outdated
@@ -22,6 +22,10 @@ pipeline {
stage('setup') {
steps {
sh '''
git diff --name-status "origin/$CHANGE_TARGET" "origin/$BRANCH_NAME" > file-list-status.log
git diff --name-only '--diff-filter=ACMRTUXB' "origin/$CHANGE_TARGET" "origin/$BRANCH_NAME" > file-list-changed.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 16, 2018

Contributor

should just be --diff-filter=d so we don't get deleted files.

This comment has been minimized.

Copy link
@dubb-b

dubb-b Oct 16, 2018

The line below should cover that correct? He is doing 2 logs 1 for status, 1 for changes, and one for deleted files.

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 16, 2018

Contributor

It should, but I would prefer using the not-operator so if anything changes, we'll be fine.

.ci/lint Outdated
else
tox -e pylint-salt ${_FILES} | tee pylint-report.xml
fi
egrep -i '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-salt | tee pylint-report-salt.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 16, 2018

Contributor

Please use grep -E.

.ci/lint Outdated
else
tox -e pylint-tests ${_FILES} | tee pylint-report-tests.xml
fi
egrep -i '^tests/.*\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-tests | tee pylint-report-tests.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 16, 2018

Contributor

grep -E again

@KaiSforza

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

Also, if we can squash some of the commits that would be great. (the ones where you just fix syntax, squash those)

@dubb-b

This comment has been minimized.

Copy link

commented Oct 16, 2018

@damon-atkins I like having all three log files archived. We could patch the server to allow/send the correct mime type. https://issues.jenkins-ci.org/browse/JENKINS-3803

What I would like to see here before we merge is some better testing ran against it.

@KaiSforza Can you please test this out on a few PR's by doing a replay or setting up some sort of test?

@KaiSforza

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

Yep I'll get on it

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

Change the names to *.log as this is configured https://github.com/jenkinsci/jenkins/blob/master/war/src/main/webapp/WEB-INF/web.xml#L225

The file contents are not xml.

@damon-atkins damon-atkins force-pushed the damon-atkins:jenkins_pylint branch from 65da679 to cf275db Oct 17, 2018

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

Should be right now. I am not a trusted user in Jenkins so its still running the old ci/lint.

@damon-atkins damon-atkins changed the title CI Lint Improvements Jenkins CI Lint Improvements Oct 17, 2018

.ci/lint Outdated
else
tox -e pylint-salt ${_FILES} | tee pylint-report.xml
fi
egrep -Ei '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-salt | tee pylint-report-salt.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 17, 2018

Contributor

not egrep -E, just grep -E.

.ci/lint Outdated
else
tox -e pylint-tests ${_FILES} | tee pylint-report-tests.xml
fi
egrep -Ei '^tests/.*\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-tests | tee pylint-report-tests.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 17, 2018

Contributor

Same as above.

Show resolved Hide resolved .ci/lint Outdated
@KaiSforza

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

I'd love these to be squashed down to one or two commits, honestly.

.ci/lint Outdated
egrep -i '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-salt | tee pylint-report-salt.log
egrep -Ei '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-salt | tee pylint-report-salt.log
# remove color escape coding
sed -ri 's/\x1B\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-salt.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 17, 2018

Contributor

okay explain to me what this is actually needed for? the colors are automatically removed when pylint is piped to something, unless something is getting through.

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Oct 17, 2018

Author Member

Save the current xml version and you will see them. I suspect PY_COLOR forces it. And is done so, so the console out put looks nice in Jenkins

@damon-atkins damon-atkins force-pushed the damon-atkins:jenkins_pylint branch from cf275db to f85fb5e Oct 18, 2018

.ci/lint Outdated
else
tox -e pylint-salt ${_FILES} | tee pylint-report.xml
fi
egrep -E '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-salt | tee pylint-report-salt.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 18, 2018

Contributor

We're still doing egrep here, please remove that first e, it should be just grep -E.

.ci/lint Outdated
else
tox -e pylint-tests ${_FILES} | tee pylint-report-tests.xml
fi
egrep -E '^tests/.*\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-tests | tee pylint-report-tests.log

This comment has been minimized.

Copy link
@KaiSforza

KaiSforza Oct 18, 2018

Contributor

Same as above.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

FYI egrep is more common use than grep -E as not all platforms support grep -E i.e. Non-Linux/Non-GNU grep, but 99% of platforms support egrep which is the same as grep -E. So when writing portable code egrep is a better choice.

@damon-atkins damon-atkins force-pushed the damon-atkins:jenkins_pylint branch from f85fb5e to 1e3c652 Oct 18, 2018

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

All updates ask for completed.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Current Open Pull Requests against 2017.7 are

  • 50128 Aug 19 Workaround for py2 builtin, =<3.3 imp and >=3.4 libimport quirks (Has Passed Lint Test)
  • 46713 Mar 27 Don't refresh modules twice per sync or refresh ops
  • 45839 Feb 2 Don't call close on singletons

So if this was merged into 2017.7 and issues occurred it would be manageable.

@dubb-b

This comment has been minimized.

Copy link

commented Oct 19, 2018

@damon-atkins I ran this against a few PR's

I didn't dive too much into the reasoning. But there seems to be some discrepancies.

https://jenkinsci.saltstack.com/view/Pull%20Requests/job/pr-lint/view/change-requests/job/PR-50124/

Notice #5 and #6

5 is a replay with the new suggested changes and 6 is the old linting pipeline code. I don't have time to dig in at the moment, although it may give you an idea as to what's wrong.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

I will have a look at then when clause, should be able to emulate it in any jenkins server, see why its not returning true.

Adjust jenkins linting process, only run stage if required, use git d…
…iff to find files, instead of find, report on status, changed and deleted files, lint only changed files.

@damon-atkins damon-atkins force-pushed the damon-atkins:jenkins_pylint branch from 1e3c652 to 6b96a24 Oct 20, 2018

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

@dubb-b that should do the job.

@damon-atkins damon-atkins force-pushed the damon-atkins:jenkins_pylint branch from b78c7ac to 5f708fa Oct 21, 2018

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

Go Go Jenkins!

@dubb-b

This comment has been minimized.

Copy link

commented Oct 22, 2018

@damon-atkins I ran it against this: https://jenkinsci.saltstack.com/view/Pull%20Requests/job/pr-lint/view/change-requests/job/PR-50152/2/

Build #1 was successful and not it fails. Looking through the logs, it seems to have found 3 warnings. Can you validate that is what you'd expect. It looks right to me.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

Looks Good.

@damon-atkins

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

So with all the recent changes to lint, the next step is only allow the other tests when the lint succeeds. This will reduce the testing load as per the example in #50026 (comment) However "Go Go Jenkins!" should override this (if possible)

@dubb-b

This comment has been minimized.

Copy link

commented Oct 22, 2018

That is definitely on the roadmap. Not sure as to when we will get it in. But thanks for the work here. Much appreciated.

@dubb-b

dubb-b approved these changes Oct 22, 2018

Copy link

left a comment

We have tested this on a number of PR's and it looks to be stable and working as it should.

@rallytime rallytime requested review from gtmanfred and terminalmage Oct 22, 2018

@gtmanfred gtmanfred merged commit 622bb51 into saltstack:2017.7 Oct 22, 2018

8 of 10 checks passed

jenkins/pr/py2-windows-2016 running py2-windows-2016...
Details
jenkins/pr/py3-windows-2016 running py3-windows-2016...
Details
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/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@damon-atkins damon-atkins deleted the damon-atkins:jenkins_pylint branch Oct 22, 2018

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.