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

Change: Rewrite Dockerhost scripts in Go #25

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

CGoodwin90
Copy link

@CGoodwin90 CGoodwin90 commented Apr 17, 2023

In conjunction with uselagoon/lagoon-charts#561

WIP - Rewrote in Go

  • docker-host/update-images.sh
  • docker-host/prune-images.sh
  • docker-host/remove-exited.sh

Moved cmd directive to execute in Go.

@CGoodwin90 CGoodwin90 marked this pull request as ready for review April 20, 2023 07:29
docker-host/main.go Outdated Show resolved Hide resolved
docker-host/main.go Outdated Show resolved Hide resolved
docker-host/main.go Show resolved Hide resolved
docker-host/main.go Show resolved Hide resolved
docker-host/main.go Show resolved Hide resolved
docker-host/main.go Outdated Show resolved Hide resolved
docker-host/main.go Outdated Show resolved Hide resolved
docker-host/main.go Outdated Show resolved Hide resolved
@shreddedbacon shreddedbacon self-requested a review May 11, 2023 00:02
Copy link
Member

@shreddedbacon shreddedbacon left a comment

Choose a reason for hiding this comment

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

Minor changes to errors, otherwise all good (considering the below around filtering)

docker-host/main.go Outdated Show resolved Hide resolved
docker-host/main.go Outdated Show resolved Hide resolved
docker-host/main.go Outdated Show resolved Hide resolved
@shreddedbacon
Copy link
Member

shreddedbacon commented May 11, 2023

I see you're just doing the filter to split by pipe. Even though the previous grep was doing regex pattern matching, splitting by pipe will solve the problem. But if other users have deployed the existing chart and are seeing that it does regex, then this filter will still fail.

I see the thought process on this, but being regex I could put uselagoon|amazeeio|*.io and the previous grep filter would work. But if I was to put that into this filter that docker is using, it would maybe not work properly?

I think as this is an entirely new method of doing filtering, the old method of pulling in the filter from the helmchart should be deprecated.

So instead of doing this we should probably change the chart to be, and make it clear these must be full repositories (or even the full filter value of *lagoon/*:*, this would probably be best honestly, then you can add any filter you want without being bound to the fmt of *%s/*:*)

repositoriesToUpdate: 
	- amazeeio
	- uselagoon
## OR
repositoriesToUpdate: 
	- amazeeio/*:*
	- *lagoon/*:*

And then updating the deployment to do something like this

        - name: REPOSITORY_TO_UPDATE
          value: {{ join "|" .Values.dockerHost.repositoriesToUpdate | quote }}

Copy link
Member

@shreddedbacon shreddedbacon left a comment

Choose a reason for hiding this comment

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

Minor thing in the dockerfile for the change to filters

docker-host/Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@shreddedbacon shreddedbacon left a comment

Choose a reason for hiding this comment

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

Seems good to me

docker-host/main.go Outdated Show resolved Hide resolved
@tobybellwood
Copy link
Member

Also seeing this message - may be worth disabling authentication check in here

time="2023-08-17T05:42:46.173119194Z" level=warning msg="Please consider generating tls certificates with client validation to prevent exposing unauthenticated root access to your network" host="tcp://0.0.0.0:2375"
time="2023-08-17T05:42:46.173220943Z" level=warning msg="You can override this by explicitly specifying '--tls=false' or '--tlsverify=false'" host="tcp://0.0.0.0:2375"
time="2023-08-17T05:42:46.173282740Z" level=warning msg="Support for listening on TCP without authentication or explicit intent to run without authentication will be removed in the next release" host="tcp://0.0.0.0:2375"

docker-host/main.go Outdated Show resolved Hide resolved
@tobybellwood tobybellwood force-pushed the change/refactor_dockerhost_scripts_golang branch from 6be93e1 to 2ac9c4f Compare August 26, 2024 23:01
@tobybellwood
Copy link
Member

ok @CGoodwin90 - this is back!

Couple of notes:

  • Instead of having the danglingFilter as a boolean option that runs after the pruneImagesSchedule - it should run on a separate schedule on its own, just to remove dangling images
  • we'll need to do a go modules update to this, and it will break some code - likely github.com/docker/docker
# docker-host
./main.go:101:54: undefined: types.ContainerListOptions
./main.go:111:59: undefined: types.ContainerRemoveOptions
./main.go:128:55: undefined: types.ImageListOptions
./main.go:148:51: undefined: types.ImagePullOptions
./main.go:161:56: undefined: types.ImageListOptions

@CGoodwin90
Copy link
Author

ok @CGoodwin90 - this is back!

Couple of notes:

  • Instead of having the danglingFilter as a boolean option that runs after the pruneImagesSchedule - it should run on a separate schedule on its own, just to remove dangling images
  • we'll need to do a go modules update to this, and it will break some code - likely github.com/docker/docker
# docker-host
./main.go:101:54: undefined: types.ContainerListOptions
./main.go:111:59: undefined: types.ContainerRemoveOptions
./main.go:128:55: undefined: types.ImageListOptions
./main.go:148:51: undefined: types.ImagePullOptions
./main.go:161:56: undefined: types.ImageListOptions

Cool, I'll take a look at it!

@shreddedbacon shreddedbacon force-pushed the change/refactor_dockerhost_scripts_golang branch from 0e4d881 to aa194c8 Compare October 3, 2024 05:28
@shreddedbacon shreddedbacon force-pushed the change/refactor_dockerhost_scripts_golang branch from aa194c8 to 98df40b Compare October 3, 2024 05:28
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.

4 participants