Move ImageName class from osbs client to utils of dockerfile parser. #139

tim-vk · 2022-11-30T18:52:52Z

Summary of what is discussed in PR #129 :

There are use cases where only specific parts of an image (returned by baseimage) needs to be edited (for example: update all the tags).
Currently dockerfile-parse does not provide any tools to perform this. A seperate "Image string parser" still needed to be added by any user of dockerfile-parse.
osbs client utils already has a perfect solution: ImageName class with possibility to edit seperate components of the imagename string.
ImageName() is a better fit for dockerfile-parse than for osbs-client.

This PR moves the ImageName class to utils of dockerfile-parse.

I did need to refactor the ImageName class slightly to get the following behavior in-line with dockerfile-parse behavior:

Return None instead of empty strings.
Do not automatically add "latest" if no tag is present.
Improve str behavior. (equality check)

I've refactored & expanded the existing tests to reflect this behavior.

Do keep in mind that these changes are breaking compared to the ImageName class contained in osbs-client.

Maintainers will complete the following section

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

tests/test_parser.py

tim-vk · 2022-12-01T19:07:44Z

@rcerven : Is it the intention to keep this repo suitable for python 2? Just to be sure, before I'm going to put too much effort in trying to get that work. I'm having quite a bit of trouble wrapping my head around what's going wrong currently.

It seems something with stringIO not accepting the result from the to_string() output. But not sure how i would be able to get it to work.

rcerven · 2022-12-01T19:24:13Z

@rcerven : Is it the intention to keep this repo suitable for python 2? Just to be sure, before I'm going to put too much effort in trying to get that work. I'm having quite a bit of trouble wrapping my head around what's going wrong currently.

It seems something with stringIO not accepting the result from the to_string() output. But not sure how i would be able to get it to work.

mmm for now afaik, @MartinBasti ?

MartinBasti · 2022-12-02T10:21:26Z

@rcerven : Is it the intention to keep this repo suitable for python 2? Just to be sure, before I'm going to put too much effort in trying to get that work. I'm having quite a bit of trouble wrapping my head around what's going wrong currently.
It seems something with stringIO not accepting the result from the to_string() output. But not sure how i would be able to get it to work.

mmm for now afaik, @MartinBasti ?

We don't need it for py2 anymore, not sure about other community users :|

tim-vk · 2022-12-04T21:34:51Z

@MartinBasti Whelp, i'm stumped: I've got everything up to snuff: Except for the python 2 bit...
I have 0 clue how I would be able to fix these failing unit tests. Do you maybe have an idea?

Disabling the python2 tests is also a possibility. but that kind of feels like the cowards' way out...

MartinBasti · 2022-12-05T10:48:56Z

@MartinBasti Whelp, i'm stumped: I've got everything up to snuff: Except for the python 2 bit... I have 0 clue how I would be able to fix these failing unit tests. Do you maybe have an idea?

Disabling the python2 tests is also a possibility. but that kind of feels like the cowards' way out...

I'm really thinking about bumping major version and drop py2 support, py2 is dead and enterprise distros can use old versions of Dockerfile-parse, I'll check with other maintainers

MartinBasti · 2022-12-07T13:03:15Z

@MartinBasti Whelp, i'm stumped: I've got everything up to snuff: Except for the python 2 bit... I have 0 clue how I would be able to fix these failing unit tests. Do you maybe have an idea?
Disabling the python2 tests is also a possibility. but that kind of feels like the cowards' way out...

I'm really thinking about bumping major version and drop py2 support, py2 is dead and enterprise distros can use old versions of Dockerfile-parse, I'll check with other maintainers

@tim-vk we agreed that we will do a major release of dockerfile-parse without py2 support. I'll drop py2 support in separate PR, so for now ignore py2 failures

MartinBasti · 2022-12-07T13:05:17Z

dockerfile_parse/parser.py

-    return match.group('image', 'name') if match else (None, None)
+    image = ImageName.parse(match.group('image')) if match else None
+    name = match.group('name') if match else None
+    return image, name


we are breaking BW compatibility here, I'm not sure if we can do this

As far as i'm aware it shouldn't break backwards compatibility:

Old version: Image was a string
New version: Image is the new ImageName class which has a str representation (which is idential to the original string of the previous version).

This should be backwards compatible (the unit tests reflect this: there are only tests added, not changed).

Addition:

Just realized what I typed above is not 100% true: See the explicit str(s) in util.py (line 51).
impact should still be minimal.

Alternative solution would be keeping the old function.

yes, explicit casting to str() is required, that's breaking change

IMO would be safer to add new function and add deprecation warning to this one

Done!
I could not find any examples within the containerbuildsystem project with deprecations so i opted to just add a simple DeprecationWarning. Let me know if an external package or other method is preferred.

Signed-off-by: Tim van Katwijk <timvankatwijk@hotmail.com>

tim-vk

move to 2 commits & finishing touches.

tim-vk · 2022-12-26T20:32:32Z

.packit.yaml

@@ -17,3 +17,7 @@ jobs:
    targets:
    - fedora-all
    - epel-8-x86_64
+
+srpm_build_deps:


This was apparently still missing in the master repo: I'm not 100% certain if this is correct. Please verify.

I'm still not sure what this fix is fixing, we had not such issue previously

Signed-off-by: Tim van Katwijk <timvankatwijk@hotmail.com>

dockerfile_parse/parser.py

+    :param from_value: string like "image:tag" or "image:tag AS name"
+    :return: tuple of the image and stage name, e.g. ("image:tag", None)
+    """
+    DeprecationWarning("Use image_name_from instead.")


Signed-off-by: Tim van Katwijk <timvankatwijk@hotmail.com>

MartinBasti · 2023-01-11T18:26:37Z

LGMT, very nice :)

rcerven · 2023-01-12T13:35:20Z

can't you avoid making those breaking changes? (osbs-client and atomic-reactor are using that) and I'd prefer to keep ImageName 100% backwards compatible

Return None instead of empty strings.
Do not automatically add "latest" if no tag is present.

tim-vk · 2023-01-18T17:24:18Z

can't you avoid making those breaking changes? (osbs-client and atomic-reactor are using that) and I'd prefer to keep ImageName 100% backwards compatible

Return None instead of empty strings.

Do not automatically add "latest" if no tag is present.

While I recognize the wish for backwards compatibility, I do think that this is a great opportunity to genuinly think about "what do we want an imagename to be". And ignore the backwards compatibility bit (especially since the repo's you mention are not forced to use this version of the ImageName class. they can still use the old one. Though I hope someone will deprecate that one so everyone can switch to the new one with time).

So first: If there is nothing. What would you rather have: an empty string? or None. I would argue that None was a great choice to be returned. This makes checks a lot easier (if(imagename.registry), is a beautiful way of checking if there is a registry).

Second: a dockerfile does not need to contain a tag. automatically filling it in with latest would be superflous (as per the docker documentation the builder does that for you).
additionally: it would mean that if you would import a perfectly fine dockerfile & save it that the parser wil have changed it by adding 'latest'. I would find that undesired behavior.

Finally, If we do want to use the argument "backwards compatibility": The imageName class as implemented in this merge request is backwards compatible with existing code. e.g.: image_from() == str(image_name_from()).

So persionally I stand by these changes compared to the imageName class in the osbs-client. They seem to me to be the right way to go.

tim-vk force-pushed the feature/imageName_class branch from f6502b9 to 6efc24a Compare November 30, 2022 18:53

tim-vk marked this pull request as draft November 30, 2022 18:58

tim-vk mentioned this pull request Nov 30, 2022

Adds tools for editing tags. #129

Closed

2 tasks

github-advanced-security bot found potential problems Nov 30, 2022

View reviewed changes

tests/test_parser.py Fixed Show fixed Hide fixed

tests/test_parser.py Fixed Show fixed Hide fixed

rcerven requested review from rcerven, ben-alkov and mkosiarc November 30, 2022 20:02

tim-vk force-pushed the feature/imageName_class branch from eb46d2a to cc33931 Compare November 30, 2022 20:42

tim-vk force-pushed the feature/imageName_class branch from c49d40d to 9736cd9 Compare December 1, 2022 19:23

tim-vk force-pushed the feature/imageName_class branch from 591af13 to 8ccd28d Compare December 4, 2022 21:27

MartinBasti reviewed Dec 7, 2022

View reviewed changes

tim-vk force-pushed the feature/imageName_class branch from 24185cf to db63b5f Compare December 26, 2022 20:13

add imageName class

7d4dfe4

Signed-off-by: Tim van Katwijk <timvankatwijk@hotmail.com>

tim-vk force-pushed the feature/imageName_class branch from 0892b5e to eda9de3 Compare December 26, 2022 20:31

tim-vk commented Dec 26, 2022

View reviewed changes

tim-vk marked this pull request as ready for review December 26, 2022 20:34

tim-vk requested review from MartinBasti and removed request for ben-alkov, rcerven and mkosiarc December 26, 2022 20:35

add srpm_build_deps for packit

a374d58

Signed-off-by: Tim van Katwijk <timvankatwijk@hotmail.com>

tim-vk force-pushed the feature/imageName_class branch 2 times, most recently from b794489 to 151f6cd Compare December 26, 2022 21:09

rename fixtures.py to conftest.py to make codeql and flake8 happy.

5fe6ebb

Signed-off-by: Tim van Katwijk <timvankatwijk@hotmail.com>

tim-vk force-pushed the feature/imageName_class branch from 151f6cd to 5fe6ebb Compare December 26, 2022 21:13

MartinBasti requested review from ben-alkov, mkosiarc and rcerven January 2, 2023 15:09

github-advanced-security bot found potential problems Jan 6, 2023

View reviewed changes

tim-vk force-pushed the feature/imageName_class branch from ff6e964 to 6b579f2 Compare January 6, 2023 11:02

add image_name_from function and add deprecation warning to image_name

4c76644

Signed-off-by: Tim van Katwijk <timvankatwijk@hotmail.com>

tim-vk force-pushed the feature/imageName_class branch from 6b579f2 to 4c76644 Compare January 6, 2023 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ImageName class from osbs client to utils of dockerfile parser. #139

Move ImageName class from osbs client to utils of dockerfile parser. #139

tim-vk commented Nov 30, 2022 •

edited

tim-vk commented Dec 1, 2022 •

edited

rcerven commented Dec 1, 2022

MartinBasti commented Dec 2, 2022

tim-vk commented Dec 4, 2022

MartinBasti commented Dec 5, 2022

MartinBasti commented Dec 7, 2022

MartinBasti Dec 7, 2022

tim-vk Dec 26, 2022 •

edited

tim-vk Dec 28, 2022

MartinBasti Jan 2, 2023

MartinBasti Jan 2, 2023

tim-vk Jan 6, 2023 •

edited

tim-vk left a comment

tim-vk Dec 26, 2022

MartinBasti Jan 11, 2023

MartinBasti commented Jan 11, 2023

rcerven commented Jan 12, 2023

tim-vk commented Jan 18, 2023

Move ImageName class from osbs client to utils of dockerfile parser. #139

Are you sure you want to change the base?

Move ImageName class from osbs client to utils of dockerfile parser. #139

Conversation

tim-vk commented Nov 30, 2022 • edited

Maintainers will complete the following section

tim-vk commented Dec 1, 2022 • edited

rcerven commented Dec 1, 2022

MartinBasti commented Dec 2, 2022

tim-vk commented Dec 4, 2022

MartinBasti commented Dec 5, 2022

MartinBasti commented Dec 7, 2022

MartinBasti Dec 7, 2022

Choose a reason for hiding this comment

tim-vk Dec 26, 2022 • edited

Choose a reason for hiding this comment

tim-vk Dec 28, 2022

Choose a reason for hiding this comment

MartinBasti Jan 2, 2023

Choose a reason for hiding this comment

MartinBasti Jan 2, 2023

Choose a reason for hiding this comment

tim-vk Jan 6, 2023 • edited

Choose a reason for hiding this comment

tim-vk left a comment

Choose a reason for hiding this comment

tim-vk Dec 26, 2022

Choose a reason for hiding this comment

MartinBasti Jan 11, 2023

Choose a reason for hiding this comment

MartinBasti commented Jan 11, 2023

rcerven commented Jan 12, 2023

tim-vk commented Jan 18, 2023

tim-vk commented Nov 30, 2022 •

edited

tim-vk commented Dec 1, 2022 •

edited

tim-vk Dec 26, 2022 •

edited

tim-vk Jan 6, 2023 •

edited