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

fix(inspect): remove defaultDomain from pkg/reference #485

Merged
merged 1 commit into from
Nov 23, 2018
Merged

fix(inspect): remove defaultDomain from pkg/reference #485

merged 1 commit into from
Nov 23, 2018

Conversation

bacongobbler
Copy link
Contributor

@bacongobbler bacongobbler commented Nov 20, 2018

Docker's pkg/reference package relies on the assumption that reference.splitDockerDomain() returns the default domain when it's not present. This triggers a bug with duffle inspect and others where duffle inspect helloworld will search for a bundle called hub.cnlabs.io/helloworld:latest. It uses hub.cnlabs.io instead of docker.io because we changed the defaultDomain field.

This hack strips out the DefaultDomain from the image reference when searching for the bundle in repositories.json.

closes #337

Signed-off-by: Matthew Fisher matt.fisher@microsoft.com

@bacongobbler
Copy link
Contributor Author

Currently looking into a cleaner fix by refactoring pkg/reference, but this gets things working for the time being.

@bacongobbler bacongobbler added this to In progress in MVP via automation Nov 20, 2018
@bacongobbler bacongobbler moved this from In progress to Needs review in MVP Nov 20, 2018
@michelleN
Copy link
Contributor

I'm getting this error on install:

duffle [bacongobbler-fix-337]$ duffle install wordpress wordpress
Error: Get https://i.hope.nobody.claims.this.domain/repositories/wordpress/tags/latest: dial tcp: lookup i.hope.nobody.claims.this.domain: no such host

Am I doing something wrong to test?

const (
// DefaultDomain is the default docker domain.
// HACK(bacongobbler): set DefaultDomain to something arbitrary that will never be used, and strip it out after calling ParseNormalizedName.
DefaultDomain = "i.hope.nobody.claims.this.domain"
Copy link
Contributor

Choose a reason for hiding this comment

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

lol

@bacongobbler bacongobbler moved this from Needs review to In progress in MVP Nov 20, 2018
@bacongobbler bacongobbler changed the title fix(inspect): strip out the default domain returned from pkg/reference WIP: fix(inspect): remove defaultDomain from pkg/reference Nov 20, 2018
@bacongobbler
Copy link
Contributor Author

going back to the drawing board, so I'm marking this as WIP for the time being

@michelleN michelleN added the WIP work in progress label Nov 21, 2018
Docker's pkg/reference package relies on the assumption that reference.splitDockerDomain() returns the default domain when it's not present. This triggers a bug with `duffle inspect` and others where `duffle inspect helloworld` will search for a bundle called `hub.cnlabs.io/helloworld:latest`.

This commit removes reference.ParseNormalizedReference from appending `defaultDomain` to the bundle name.

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>
@bacongobbler bacongobbler changed the title WIP: fix(inspect): remove defaultDomain from pkg/reference fix(inspect): remove defaultDomain from pkg/reference Nov 22, 2018
@bacongobbler bacongobbler removed the WIP work in progress label Nov 22, 2018
@bacongobbler
Copy link
Contributor Author

okay, I've removed the concept of a defaultDomain from pkg/reference and all seems to work again.

PS C:\Users\me\code\go\src\github.com\deis\duffle\examples\helloworld> duffle build .
[...]
==> Successfully built bundle helloworld:0.1.0
PS C:\Users\me\code\go\src\github.com\deis\duffle\examples\helloworld> duffle inspect helloworld:0.1.0
{
    "name": "helloworld",
...
}

} else {
domain, remainder = name[:i], name[i+1:]
}
return
}

// familiarizeName returns a shortened version of the name familiar
Copy link
Member

Choose a reason for hiding this comment

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

So much red ❤️

@bacongobbler bacongobbler moved this from In progress to Needs review in MVP Nov 22, 2018
Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

Manually tested, LGTM

MVP automation moved this from Needs review to Reviewer approved Nov 23, 2018
@bacongobbler bacongobbler merged commit ede9e7d into cnabio:master Nov 23, 2018
MVP automation moved this from Reviewer approved to Done Nov 23, 2018
@bacongobbler bacongobbler deleted the fix-337 branch November 23, 2018 16:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
MVP
  
Done
Development

Successfully merging this pull request may close these issues.

Build then push fails with confusing error
3 participants