Skip to content

Conversation

@dmcgowan
Copy link
Owner

PR to review reference changes in distribution package with corresponding changes in docker engine.

See distribution/distribution#1778

dmcgowan added 2 commits June 21, 2016 21:36
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
@aaronlehmann
Copy link

I took a quick look over this and I'm pretty happy with it. My one concern is that the upstream distribution change doesn't use types to differentiate normalized references from unnormalized ones. For example, I believe the pull/push code expects a normalize reference, but since it takes reference.Named, there's nothing to stop someone from passing un an unnormalized reference, which won't work correctly. I could see people getting confused about whether to use NormalizedName or ParseNamed and getting it wrong. What do you think about defining a normalized reference type that embeds Namedand using that throughout the engine in places where we expect normalized references?

@dmcgowan dmcgowan force-pushed the upstream-reference-updates branch from 6d78732 to 2b2be6a Compare June 23, 2016 18:14
@dmcgowan
Copy link
Owner Author

What I found in doing the code changes is that ParseNamed is not currently used. All input from strings is treated as input needing normalization. Because of this, all reference.Named are normalized, which I think is good to enforce in the code. The annoying part though is that all input needs normalization and all errors, message, and storage are using the familiarized form. Meaning there are tons of NormalizedName and FamiliarName calls. I prefer having the code less ambiguous, which these explicit calls help with, but it would be great if we found a better way.

If we were to have NormalizedName as an interface, what would you expect the behavior of the different functions to be. Such as, would String() return a normalized or familiar string? I think you are right in that we should focus on ensuring that this change does not further cause confusion, but rather my hope is that the code will be much easier to understand and not leave future editors being confused about whether a value is Normalized and which function should be called to parse and stringify.

@dmcgowan
Copy link
Owner Author

Added another commit which familiarizes the output strings, still have more work to get tests passing. Most the code today assumes values are not normalized which makes this switch to having all references normalized a larger change.

@aaronlehmann
Copy link

If we were to have NormalizedName as an interface, what would you expect the behavior of the different functions to be. Such as, would String() return a normalized or familiar string?

Maybe instead of embedding reference.Named, it should be an interface like this:

type NormalizedName interface {
        Normalized() Named
        Familiar() Named
}

This would keep things explicit, and also address my concern about accidentally passing an unnormalized reference to a function expecting a normalized one or vice versa, by making a normalized name an explicit type.

@dmcgowan dmcgowan closed this Apr 4, 2019
dmcgowan pushed a commit that referenced this pull request Jul 17, 2025
    #27 94.97 executor/oci/internal/resolvconf/resolvconf.go:461:6: the error type name `systemErr` should conform to the `xxxError` format (errname)
    #27 94.97 type systemErr struct{ error }
    #27 94.97      ^

Also fix an unhandled error; we don't need a defer() for that one

Signed-off-by: Sebastiaan van Stijn <[email protected]>
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