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

Scan plugin #528

Closed
wants to merge 3 commits into from
Closed

Scan plugin #528

wants to merge 3 commits into from

Conversation

gtardif
Copy link
Contributor

@gtardif gtardif commented Mar 11, 2021

Add scan CLI plugin in CE packaging

@gtardif gtardif marked this pull request as draft March 11, 2021 13:04
@gtardif gtardif force-pushed the scan_plugin branch 4 times, most recently from a37ac00 to e98d060 Compare March 11, 2021 16:38
@gtardif gtardif marked this pull request as ready for review March 17, 2021 12:43
glours
glours previously approved these changes Mar 17, 2021
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

silvin-lubecki
silvin-lubecki previously approved these changes Mar 17, 2021
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@silvin-lubecki
Copy link
Contributor

PTAL @thaJeztah

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments; we should also squash the commits

Comment on lines 54 to 59
yum install -y http://opensource.wandisco.com/centos/7/git/x86_64/wandisco-git-release-7-2.noarch.rpm
yum install -y git
fi
git version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a custom git install? I see git is already in the BuildRequires;

I see the commit mentions "really old version by default that messes go mod download" do you have more details on that? That sounds like a serious issue with Golang if it cannot work with a major distro such as RHEL / CentOS 7

Copy link
Contributor Author

@gtardif gtardif Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more specifically, go mod download is failing because git version on centos7 is 1.8 (anything before git 2.16 is EOL from https://en.wikipedia.org/wiki/Git#Releases). Updating git allows go mod download to succeed, no further issues compiling.
See golang/go#38373 for the go issue and solution.
I'll update the PR to mention this comment for better documentation in the code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anything before git 2.16 is EOL from https://en.wikipedia.org/wiki/Git#Releases

Yes, upstream releases are EOL, but the CentOS/RHEL versions of Git 1.8 are still maintained (it's kinda horrible, but that's what makes RHEL/CentOS popular as an LTS distro).

I want to avoid installing a non-standard git package in our build-pipeline; especially downloaded over a non-TLS connection. Also not sure if they changed, but I recall (long time ago; when SVN was common) some of the wandisco installers included tracking, which I also want to avoid. (git itself mentions https://repo.ius.io, which could be an alternative).

However, I did a bit more digging for alternatives; the Dockerfile currently uses GOPROXY=direct, which (IIRC) was added to allow building from private forks for security releases;

I did some testing and if we switch to the default (GOPROXY=https://proxy.golang.org), Go won't use git to fetch the modules, making the build pass; I pushed a "draft" PR with that change; #530, but can push to your branch if you prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, I took your update and included GOPROXY here instead of updating git (Indeed I understand we don't want to change the installed packages for the entire CI pipeline). Squashed and added on top of updated scan-cli-plugin version, that simplifies make invocation

plugins/scan.installer Outdated Show resolved Hide resolved
@gtardif gtardif dismissed stale reviews from silvin-lubecki and glours via 8a2317e March 23, 2021 14:15
@gtardif gtardif force-pushed the scan_plugin branch 2 times, most recently from 8a2317e to 07f33e6 Compare March 23, 2021 14:29
@thaJeztah thaJeztah mentioned this pull request Mar 23, 2021
@gtardif gtardif force-pushed the scan_plugin branch 2 times, most recently from 3bb7d99 to af2bf79 Compare March 24, 2021 09:41
@thaJeztah
Copy link
Member

could you do one more rebase, so that it runs with #532 which was just merged? (cross compile failed in our internal release-packaging repo, and may need some additional changes)

possibly we'd also need something like #531 in the go build

glours and others added 2 commits March 24, 2021 12:26
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Alternative solution was to update git on centos7 in /rpm/SPEC/docker-ce-cli.spec: 
```
if [ "$SUITE" == "7" ]; then
    yum install -y http://opensource.wandisco.com/centos/7/git/x86_64/wandisco-git-release-7-2.noarch.rpm
    yum install -y git
fi
```

But rather not changing what is installed on the CEnTOS bistro for the entire CI pipeline

Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
@gtardif
Copy link
Contributor Author

gtardif commented Mar 24, 2021

👍 rebased

Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants