Skip to content
This repository has been archived by the owner on May 9, 2023. It is now read-only.

[KOGITO-8121]-Clean up images used by Kaniko in the docker registry #10

Merged
merged 1 commit into from
Dec 12, 2022
Merged

[KOGITO-8121]-Clean up images used by Kaniko in the docker registry #10

merged 1 commit into from
Dec 12, 2022

Conversation

desmax74
Copy link
Member

@desmax74 desmax74 commented Nov 3, 2022

See:https://issues.redhat.com/browse/KOGITO-8121
Signed-off-by: desmax74 mdessi@redhat.com

Description of the change:

Motivation for the change:

Checklist

  • Add or Modify a unit test for your change
  • Have you verified that tall the tests are passing?

@desmax74 desmax74 marked this pull request as draft November 3, 2022 15:12
@desmax74 desmax74 force-pushed the kogito-8121 branch 2 times, most recently from 22df9bd to 71139e9 Compare November 10, 2022 15:29
@desmax74 desmax74 marked this pull request as ready for review November 10, 2022 15:29
spolti
spolti reviewed Nov 11, 2022
View reviewed changes
cleaner/docker/cleaner.go Outdated Show resolved Hide resolved
spolti
spolti reviewed Nov 11, 2022
View reviewed changes
Copy link
Member

@spolti spolti left a comment

Choose a reason for hiding this comment

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

Hi Max, one question, where exactly these images will be store?
Is it working for k8s/ocp embedded registry?

or is it just a tool for cleaning local registries?

@desmax74
Copy link
Member Author

@spolti I'm starting with local registry then I'll test with remote, but I'm expecting that the only change is the credentials and url of the docker/podman registry

@desmax74
Copy link
Member Author

jenkins test this

ricardozanini
ricardozanini reviewed Nov 14, 2022
View reviewed changes
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few comments if you don't mind!

cleaner/docker/cleaner.go Outdated Show resolved Hide resolved
cleaner/podman/cleaner.go Outdated
// For example tcp://localhost:<port>
// or unix:///run/podman/podman.sock
// or ssh://<user>@<host>[:port]/run/podman/podman.sock?secure=True
func GetRemoteConnection(uri string, identity string) (context.Context, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we consider adding an interface for these features? Podman and Docker have a similar API, and I believe we can leverage that.

Copy link
Member Author

@desmax74 desmax74 Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, we can unify using some converter methods to expose only strings or boolean where the results provides docker or podman specific entities

Copy link
Member

Choose a reason for hiding this comment

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

Hi! I don't think we need additional packages for this implementation. I'd do:

cleaner
- cleaner.go
- podman.go
- docker.go

@spolti
Copy link
Member

spolti commented Nov 14, 2022

@spolti I'm starting with local registry then I'll test with remote, but I'm expecting that the only change is the credentials and url of the docker/podman registry

for local registry isn`t better just add some instructions in the readme?
I mean, for my use case, I have a few aliases to clean the registry based on image name, tags, dangling, none, etc.

@ricardozanini
Copy link
Member

@spolti I believe local is needed once the CLI starts using it.

@desmax74 desmax74 force-pushed the kogito-8121 branch 3 times, most recently from afbe106 to 49e71ca Compare November 15, 2022 10:19
@desmax74
Copy link
Member Author

@spolti I'll add the env to set to use local or remote registry

spolti
spolti suggested changes Nov 16, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
cleaner/podman/cleaner.go Outdated Show resolved Hide resolved
@desmax74 desmax74 force-pushed the kogito-8121 branch 4 times, most recently from 1198d28 to 037566b Compare November 17, 2022 20:15
spolti
spolti reviewed Nov 21, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
spolti
spolti reviewed Nov 21, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
spolti
spolti reviewed Dec 2, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
spolti
spolti reviewed Dec 2, 2022
View reviewed changes
cleaner/cleaner.go Outdated Show resolved Hide resolved
spolti
spolti reviewed Dec 2, 2022
View reviewed changes
cleaner/docker.go Outdated
return images
}

func (d Docker) GetImagesFiltered(args filters.Args) ([]types.ImageSummary, error) {
Copy link
Member

Choose a reason for hiding this comment

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

imho this method and the above one can be combined.

Copy link
Member Author

@desmax74 desmax74 Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, merged into
GetImages(args types.ImageListOptions)

GetAllImages can be called using
d.GetImages(types.ImageListOptions{All: true})

GetImageFiltered can be called using

filters := filters.NewArgs()
filters.Add("dangling", "true")
images, err := d.GetImages(types.ImageListOptions{Filters: filters})

spolti
spolti reviewed Dec 2, 2022
View reviewed changes
cleaner/docker.go Outdated Show resolved Hide resolved
spolti
spolti reviewed Dec 2, 2022
View reviewed changes
cleaner/docker.go Outdated Show resolved Hide resolved
spolti
spolti reviewed Dec 2, 2022
View reviewed changes
registry/test/podman.go Outdated Show resolved Hide resolved
@desmax74 desmax74 force-pushed the kogito-8121 branch 18 times, most recently from 6498a27 to 3aa8e7d Compare December 7, 2022 13:44
@desmax74
[KOGITO-8121]-Clean up images used by Kaniko in the docker registry
c00d0e3 
Signed-off-by: desmax74 <mdessi@redhat.com>
@desmax74 desmax74 merged commit a08d306 into kiegroup:main Dec 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@spolti spolti spolti requested changes

@ricardozanini ricardozanini ricardozanini approved these changes

@davidesalerno davidesalerno davidesalerno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@desmax74 @spolti @ricardozanini @davidesalerno