Move ImageName class from osbs client to utils of dockerfile parser.
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
@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 : 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 ?
@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 :|
@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 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 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
LGMT, very nice :)
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.
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.