Jenkins CI Lint Improvements #50026

damon-atkins · 2018-10-13T08:43:46Z

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 · 2018-10-13T13:42:58Z

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)

KaiSforza · 2018-10-16T13:23:44Z

.ci/lint

@@ -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


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

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

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

KaiSforza · 2018-10-16T13:25:03Z

.ci/lint

-                        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


Please use grep -E.

KaiSforza · 2018-10-16T13:25:15Z

.ci/lint

-                        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


grep -E again

KaiSforza · 2018-10-16T13:25:49Z

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

dubb-b · 2018-10-16T14:59:17Z

@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 · 2018-10-16T16:42:15Z

Yep I'll get on it

damon-atkins · 2018-10-17T09:05:11Z

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 · 2018-10-17T13:07:59Z

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

KaiSforza · 2018-10-17T16:39:40Z

.ci/lint

-                        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


not egrep -E, just grep -E.

KaiSforza · 2018-10-17T16:39:48Z

.ci/lint

-                        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


Same as above.

.ci/lint

KaiSforza · 2018-10-17T16:48:41Z

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

KaiSforza · 2018-10-17T16:51:38Z

.ci/lint

-                        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


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.

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

KaiSforza · 2018-10-18T09:07:08Z

.ci/lint

-                        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


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

KaiSforza · 2018-10-18T09:07:20Z

.ci/lint

-                        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


Same as above.

damon-atkins · 2018-10-18T12:20:42Z

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 · 2018-10-18T12:47:38Z

All updates ask for completed.

damon-atkins · 2018-10-19T12:13:11Z

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 · 2018-10-19T14:29:49Z

@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 · 2018-10-19T15:28:17Z

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.

…iff to find files, instead of find, report on status, changed and deleted files, lint only changed files.

damon-atkins · 2018-10-20T14:33:04Z

@dubb-b that should do the job.

damon-atkins · 2018-10-22T13:42:58Z

Go Go Jenkins!

dubb-b · 2018-10-22T15:29:29Z

@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 · 2018-10-22T15:49:10Z

Looks Good.

damon-atkins · 2018-10-22T15:53:06Z

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 · 2018-10-22T15:57:27Z

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

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

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

gtmanfred requested a review from KaiSforza October 14, 2018 00:21

cachedout requested a review from dubb-b October 16, 2018 09:30

KaiSforza suggested changes Oct 16, 2018

View reviewed changes

damon-atkins force-pushed the jenkins_pylint branch from 65da679 to cf275db Compare October 17, 2018 13:03

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

KaiSforza suggested changes Oct 17, 2018

View reviewed changes

KaiSforza reviewed Oct 17, 2018

View reviewed changes

damon-atkins force-pushed the jenkins_pylint branch from cf275db to f85fb5e Compare October 18, 2018 08:04

KaiSforza suggested changes Oct 18, 2018

View reviewed changes

damon-atkins force-pushed the jenkins_pylint branch from f85fb5e to 1e3c652 Compare October 18, 2018 12:20

KaiSforza approved these changes Oct 18, 2018

View reviewed changes

Adjust jenkins linting process, only run stage if required, use git d…

6b96a24

…iff to find files, instead of find, report on status, changed and deleted files, lint only changed files.

damon-atkins force-pushed the jenkins_pylint branch from 1e3c652 to 6b96a24 Compare October 20, 2018 14:31

use awk to create the other files, so git does not need to search again

5f708fa

damon-atkins force-pushed the jenkins_pylint branch from b78c7ac to 5f708fa Compare October 21, 2018 09:07

Merge branch '2017.7' into jenkins_pylint

aebce88

dubb-b approved these changes Oct 22, 2018

View reviewed changes

rallytime requested review from gtmanfred and terminalmage October 22, 2018 19:05

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

damon-atkins deleted the jenkins_pylint branch October 22, 2018 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jenkins CI Lint Improvements #50026

Jenkins CI Lint Improvements #50026

damon-atkins commented Oct 13, 2018 •

edited

Loading

damon-atkins commented Oct 13, 2018 •

edited

Loading

KaiSforza Oct 16, 2018

dubb-b Oct 16, 2018

KaiSforza Oct 16, 2018

KaiSforza Oct 16, 2018

KaiSforza Oct 16, 2018

KaiSforza commented Oct 16, 2018

dubb-b commented Oct 16, 2018

KaiSforza commented Oct 16, 2018

damon-atkins commented Oct 17, 2018

damon-atkins commented Oct 17, 2018

KaiSforza Oct 17, 2018

KaiSforza Oct 17, 2018

KaiSforza commented Oct 17, 2018

KaiSforza Oct 17, 2018

damon-atkins Oct 17, 2018

KaiSforza Oct 18, 2018

KaiSforza Oct 18, 2018

damon-atkins commented Oct 18, 2018 •

edited

Loading

damon-atkins commented Oct 18, 2018

damon-atkins commented Oct 19, 2018

dubb-b commented Oct 19, 2018

damon-atkins commented Oct 19, 2018

damon-atkins commented Oct 20, 2018

damon-atkins commented Oct 22, 2018

dubb-b commented Oct 22, 2018

damon-atkins commented Oct 22, 2018

damon-atkins commented Oct 22, 2018 •

edited

Loading

dubb-b commented Oct 22, 2018

dubb-b left a comment

Jenkins CI Lint Improvements #50026

Jenkins CI Lint Improvements #50026

Conversation

damon-atkins commented Oct 13, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

damon-atkins commented Oct 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaiSforza commented Oct 16, 2018

dubb-b commented Oct 16, 2018

KaiSforza commented Oct 16, 2018

damon-atkins commented Oct 17, 2018

damon-atkins commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaiSforza commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damon-atkins commented Oct 18, 2018 • edited Loading

damon-atkins commented Oct 18, 2018

damon-atkins commented Oct 19, 2018

dubb-b commented Oct 19, 2018

damon-atkins commented Oct 19, 2018

damon-atkins commented Oct 20, 2018

damon-atkins commented Oct 22, 2018

dubb-b commented Oct 22, 2018

damon-atkins commented Oct 22, 2018

damon-atkins commented Oct 22, 2018 • edited Loading

dubb-b commented Oct 22, 2018

dubb-b left a comment

Choose a reason for hiding this comment

damon-atkins commented Oct 13, 2018 •

edited

Loading

damon-atkins commented Oct 13, 2018 •

edited

Loading

damon-atkins commented Oct 18, 2018 •

edited

Loading

damon-atkins commented Oct 22, 2018 •

edited

Loading