-
Notifications
You must be signed in to change notification settings - Fork 75
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
Bump overall dependencies #161
Conversation
|
@superseb thank you for the review. Given the size of the PR and the moving parts, do you think those changes can be tackled as a follow-up PR, or should I just add to this? |
@superseb removed Go mod vendoring. PTAL |
@pjbgf Do you have an ETA for when Seb's review comments will be addressed? We need a new tag with |
All the changes were made, @superseb @kinarashah. PTAL |
Dockerfile.dapper
Outdated
DOCKER_URL=DOCKER_URL_${ARCH} | ||
|
||
RUN wget -O - ${!DOCKER_URL} > /usr/bin/docker && chmod +x /usr/bin/docker | ||
RUN go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.52.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why deviate from the default binary install like in every other project? Based on their own docs, go get
/go install
is discouraged (https://golangci-lint.run/usage/install/#install-from-source). This also blocks auto updating because it doesn't match our generic rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change was aligned with the current version in master which uses go get
.
This has now changed to use upstream's install.sh
instead.
package/Dockerfile
Outdated
FROM nginx:1.21.6-alpine | ||
FROM nginx:1.24.0-alpine | ||
|
||
ENV DOCKER_VERSION=23.0.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this tested to be compatible with the oldest Docker we support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not completely sure what version we do support, but generally that is defined via DOCKER_API_VERSION
, which we are not setting in this project apart from the entrypoint.sh
. What API version is this project meant to support?
@kinarashah can you please help testing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can override the rke-tools image to test using the system_images
option in cluster.yml (CLI only)
Basically comes down to API support for the versions we do support (see support matrix)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned this with support matrix, and bumped to latest patch: 20.10.25
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with 23.0.3
as long as its API compatible with the versions we support. But in the PR I am viewing it still says 23.0.3
while you say you bumped it to 20.10.25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice spot, that is now fixed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the docker within the 20.x
series as that is aligned with other projects and our support matrix.
The choice of 20.10.24
over 20.10.25
, was due to although the project has a tag for 20.10.25
, it does not have the artefacts published for them.
One final question is if we are going for Go 1.20 in 2.7 Q2 release as we are moving to k8s 1.26 which is 1.19, do we want to keep that version across the board or is it up to the teams? |
Is it possible to know when this will be merged ? |
0e0c861
to
7936f85
Compare
@kpsingh219 I am not entirely sure. It will largely dependent on reviews and testing. |
I think the main constraint is running supported version only. Apart from that, I would assume that's up to the teams/projects. |
ecd9765
to
aa14d98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving from a pure security perspective regarding the bumps. We still need @kinarashah's review and probably tag someone from QA to test all the pieces together.
package/Dockerfile
Outdated
FROM nginx:1.24.0-alpine | ||
|
||
ENV DOCKER_VERSION=23.0.3 | ||
ENV PLUGINS_VERSION=v1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a nit, but naming it more specifically allows us to put this automated bump in Renovate with a more descriptive variable (for example CNI_PLUGINS_VERSION
instead of PLUGINS_VERSION
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good shout, amended it accordingly.
2374c1b
to
f7721f7
Compare
Note that CNI plugins v0.9.1 included flannel binary but |
@manuelbuil thank you for pointing that out, that should be fixed now. PTAL I noticed that all the plugins are being kept on |
You are pointing to |
@manuelbuil That was intentional, as that was I trying to highlight the current version on |
Following up from off-line chat with @manuelbuil, we will keep binaries as per current public image. If necessary, a future PR will move things around. I added a test to verify all binaries being packaged up are in place and are valid for the target architecture. |
The build is failing as |
- github.com/minio/minio-go/v7 to version 7.0.52. - github.com/sirupsen/logrus to version 1.9.0. - github.com/urfave/cli to version 1.22.12. Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
It looks like the rancher fork of plugins was created in order to add arm64 support. Given that upstream supports that architecture natively nowadays, this has been removed. Ref: rancher/plugins#2 Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
Moved all the environment var declarations to the top of the Dockerfile, for easier maintainance going forwards. Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
Migrate from the deprecated golint to golangci-lint + revive, and during the process the depracted io/ioutil package was also migrated. Instead of manually downloading the docker CLI within dapper, that is now installed with zypper instead. Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
This change was needed as flannel was previously baked into the cni plugins, but this is no longer the case. Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
Verify expected binaries on the target image, ensuring they are valid for the target architecture. Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
The previous version had an invalid arm64 image, this bump should fix it. Signed-off-by: Paulo Gomes <paulo.gomes@suse.com>
PR rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good to me.
@pjbgf please create an issue which can be used to QA this PR |
Summary of changes:
v0.6.0
.v1.24.0
.v1.24.15
.rancher/plugins
in favour of the upstream latest version.rancher/docker
in favour of the upstream latest version.