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

Adds tools for editing tags. #129

Closed

Conversation

tim-vk
Copy link
Contributor

@tim-vk tim-vk commented Sep 28, 2022

Adds some functions for getting & setting image tags.
Adds a 'basetag' property for quickly adding/changing the tag of the baseimage.

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered

@tim-vk tim-vk force-pushed the feature/version_tags branch 3 times, most recently from 632798d to fe277cb Compare October 1, 2022 10:55
@tim-vk tim-vk changed the title Feature/version tags Adds tools for editing tags. Oct 3, 2022
@tim-vk
Copy link
Contributor Author

tim-vk commented Oct 6, 2022

@rcerven , @twaugh . Maybe a rude question but: is this project still alive? (Or am I just impatient ;) ).

@rcerven
Copy link
Member

rcerven commented Oct 6, 2022

@rcerven , @twaugh . Maybe a rude question but: is this project still alive? (Or am I just impatient ;) ).

Hi, yes project is alive, we are just busy with osbs2

Copy link
Contributor

@MartinBasti MartinBasti left a comment

Choose a reason for hiding this comment

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

Hello,

can you please describe describe the use case little bit more?

From my point of view, tag is tied with image, and not directly Dockerfile, for me tag is attribute of image and not direct dependency of dockerfile.

IMO better solution would be pass image string into a "ImageNameParser" and return tag form there, to keep responsibility separated.

@@ -401,7 +401,7 @@ def parent_images(self, parents):
lines[instr['startline']:instr['endline']+1] = [instr['content']]

self.lines = lines

Copy link
Contributor

Choose a reason for hiding this comment

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

This change shouldn't be here

Copy link
Contributor Author

@tim-vk tim-vk Nov 22, 2022

Choose a reason for hiding this comment

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

can you please describe describe the use case little bit more?

The use case I have: I have a dockerfile with images & tags (that are not "latest" since we want to pin our images to specific tags).
Every once in a while I want to (semi-automatically) update these tags within the dockerfile. e.g.:

  • Load dockerfile
  • Get base image
  • Update base image to latest (or other specific) tag (0.1.0 -> 0.2.0).

While dockerfile-parse has the tools to load the dockerfile and get the image it was lacking tools to split up this image in it's base components (registry\image:tag). Currently I'm using this in a seperate script but I doubt i'm the only person that would like to just update the tags of an image.

From my point of view, tag is tied with image, and not directly Dockerfile, for me tag is attribute of image and not direct dependency of dockerfile.

My personal use case is heavily "tag" focussed. The way I see it tag is a seperate imput for the FROM keyword (as per the docker documentation

IMO better solution would be pass image string into a "ImageNameParser" and return tag form there, to keep responsibility separated.

I think the first thing we'd need to agree apon is: "Is the tag part of the image name"?

If no: Then a seperate "ImageNameParser" is the better structure. (Would this be a seperate repo?).
If yes: Then I think this PR would be in the right direction.

Edit:

I was looking back at my other PR and remembered something:

Ideally I would change the entire "image_from" function to a "parse_FROM" function which returns all values that you could give the FROM keyword (platform, image, digest, tag and name). But that would have been massively breaking (since this entire project assumes "tag" is part of your image, and not a seperate entity). So I opted for this solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, dockerfile-parse works with imagename as a whole including digest or tag.

I'm not very eager to make it more granular, because we may also split imagename into: registry, namespace, image, tag/digest. There are multiple parts, many of them optional.

I'd like to keep dockerfile parse simple and rather use 3rd party library to manipulate with image string and just pass it to dockerfile parse

Copy link
Contributor

Choose a reason for hiding this comment

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

We are using it like that, dockerfile just provides raw string, we manipulate it with separate code
https://github.com/containerbuildsystem/osbs-client/blob/master/osbs/utils/__init__.py#L372

Copy link
Contributor

Choose a reason for hiding this comment

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

This is naming convention we use

    Naming Conventions
    ==================
    registry.somewhere/namespace/image_name:tag
    |-----------------|                          registry, reg_uri
                      |---------|                namespace
    |--------------------------------------|     repository
                      |--------------------|     image name
                                            |--| tag
                      |------------------------| image
    |------------------------------------------| image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a specific 3rd party "image parser" that you know of that works with this convention?

I find it odd that osbs-client (your link) contains the logic to split up the image into registry, tag, name etc. which seems to be a bit counterintuitive with the "keep responsibility seperated" mentality.

The thing that I apparently am running against is that this repo doesn't seem to be a generic "dockerfile parser" but an "osbs dockerfile parser" which decreases the usability of it somewhat.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow what do you mean.

What I want to avoid, is having separate method for everything: get_tag, set_tag, get_registry, set_registry, etc..

We are modifying registry part, we didn't put that code into docker_file parse, because it's better to have single class for manipulating with imagenames and then just write back string representation into dockerfile parse.

This also work for all stages in Dockerfile, not just final stage.

What could be good, is to modify dockerfile-parse to return imagename as object where you can use methods to modify image, or make this more object oriented, and return object per stage.

I'd like to keep here some OOP decoupling between image and dockerfile statements

Copy link
Contributor Author

@tim-vk tim-vk Nov 26, 2022

Choose a reason for hiding this comment

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

I think I'm starting to get your intention.

But if you'd truly want to decouple dockerfile statements I would expect something like get_FROM_arguments() Which would return all the possible arguments that are allowed for the FROM statement (see dockerfile reference). Or a class (FromArguments) where you can set the different arguments.

Regardless of the approach. What I don't really get is: Why isn't the ImageName class included in osbs-client? And not in dockerfile-parse? (or why isn't it a seperate repo?).

The thing thats bugging the most in the current set-up is that being able to change just the tag (or just the registry or other seperate component of "image") seems to be a fairly common & normal use case. And to achieve that either needs you to add your own functions (which I am sharing here) or install osbs-client (which doesn't seem to be available via pip?). Which raises my earlier question: Is dockerfile-parse meant as just a supplementary tool for osbs-client or as a tool for anyone who want to parse dockerfiles (and nothing else).

Edit:
Just spitballing:

Would it be more fitting for this project if the ImageName class from osbs-client was moved to util.py from this project?

Or these tag functions to util.py?

Copy link
Contributor

@MartinBasti MartinBasti Nov 28, 2022

Choose a reason for hiding this comment

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

Why isn't the ImageName class included in osbs-client? And not in dockerfile-parse? (or why isn't it a seperate repo?).

Historical reasons, I don't know. Before it was part of atomic-reactor.

Would it be more fitting for this project if the ImageName class ...

This would work for me, sounds like great idea. Let's wait what other maintainers things about it.

Copy link
Member

Choose a reason for hiding this comment

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

moving ImageName here and returning base/parent images with it sounds good to me

@tim-vk
Copy link
Contributor Author

tim-vk commented Nov 30, 2022

Since I'm using this specific branch for personal projects (and i don't want to update them all at once) I've created a new branch and PR: #139.

Closing this PR.

@tim-vk tim-vk closed this Nov 30, 2022
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.

3 participants