<p style="white-space: pre-wrap; word-wrap: break-word;">Thanks, I think this is a good improvement.</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-ci/+/26393">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-ci/+/26393/1//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ci/+/26393/1//COMMIT_MSG@13">Patch Set #1, Line 13:</a> <code style="font-family:monospace,monospace">The jenkins nodes needs to access the gerrit via ssh</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I would expect one can createa gerrit user that has permission only to provide review and thereby limit the potential damage.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, I would recommend this even if we can ensure that only reviewed osmo-ci.git code gets ssh access.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">the gerrit-lint isn't a problem to give ssh access since it will only execute reviewed code from osmo-ci. So we could just add ssh-key to jenkins and enable ssh-agent in jenkins.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Currently there's an exception: when modifying osmo-ci.git, the linter code gets executed from the repository state submitted to gerrit:</p><p style="white-space: pre-wrap; word-wrap: break-word;">https://git.osmocom.org/osmo-ci/tree/jobs/gerrit-lint.yml?id=cccd0cdd084a68b67d22a903540d4a699ac376db#n32</p><p style="white-space: pre-wrap; word-wrap: break-word;">So that needs to be removed. Probably best to remove the "cmd" variable from https://git.osmocom.org/osmo-ci/tree/jobs/gerrit-lint.yml?id=cccd0cdd084a68b67d22a903540d4a699ac376db#n8 to avoid setting another cmd in the future that points to the repository modified in gerrit, without thinking about ssh access the script would get. I suggest we put cmd here instead https://git.osmocom.org/osmo-ci/tree/jobs/gerrit-lint.yml?id=cccd0cdd084a68b67d22a903540d4a699ac376db#n90 so it cannot be overridden per project.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-ci/+/26393/1/lint/lint_diff.sh">File lint/lint_diff.sh:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ci/+/26393/1/lint/lint_diff.sh@34">Patch Set #1, Line 34:</a> <code style="font-family:monospace,monospace">if ! git diff -U0 "$COMMIT" | "$SCRIPT_DIR/checkpatch/checkpatch_osmo.sh" > gerrit_report ; then</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This condition doesn't need to check if errors were found or not, because above it does "exit 0" if there are no errors. But this should not run by default, only if sending comments to gerrit is explicitly enabled. So it does not run if used locally as git pre-commit hook.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I suggest changing this to:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if [ -n "$GERRIT_COMMENT" ]; then<br>    git diff -U0 "$COMMIT" | "$SCRIPT_DIR/checkpatch/checkpatch_osmo.sh" > gerrit_report</pre><p style="white-space: pre-wrap; word-wrap: break-word;">And in jobs/gerrit-lint.yml, add GERRIT_COMMENT=1 before calling lint_diff.sh here: https://git.osmocom.org/osmo-ci/tree/jobs/gerrit-lint.yml?id=cccd0cdd084a68b67d22a903540d4a699ac376db#n8</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-ci/+/26393">change 26393</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-ci/+/26393"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-ci </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I1a48ddb976e0f53bfc0552d0be11e42ba68d9e49 </div>
<div style="display:none"> Gerrit-Change-Number: 26393 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 29 Nov 2021 09:00:35 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>