Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Protoype: allow constraint.source to be a sub-package of an repo #1000

Closed
wants to merge 1 commit into from

Conversation

sttts
Copy link

@sttts sttts commented Aug 12, 2017

This prototypes to have non-root packages as constraint.source values:

[[override]]
  name = "k8s.io/apimachinery"
  source = "github.com/sttts/k8s-dep-e2e/internal/_staging/k8s.io/apimachinery"

Compare fabulous-gopher/k8s-dep-e2e#7 as an example repo.

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

hmmm.

so, i'm quite dubious about this, because the invariant that roots are never nested is one that i just accepted quite some time ago, and i've stopped keeping mental notes on what implicitly relies on that invariant. all i have is vague, placeless memories, deep in the weeds of some logic that "boy, this really locks us out from nested roots."

but, this is a more limited scope way of driving at that, and it's possible that it's not as harmful as doing it in general. so, i'm keeping an open mind 😄

@@ -548,7 +552,7 @@ type source interface {
updateLocal(context.Context) error
listVersions(context.Context) ([]PairedVersion, error)
getManifestAndLock(context.Context, ProjectRoot, Revision, ProjectAnalyzer) (Manifest, Lock, error)
listPackages(context.Context, ProjectRoot, Revision) (pkgtree.PackageTree, error)
listPackages(context.Context, string, ProjectRoot, Revision) (pkgtree.PackageTree, error)
Copy link
Member

Choose a reason for hiding this comment

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

this strikes me as unnecessary - it's a fairly straightforward manipulation to select a subtree of a PackageTree. as such, this could just be a method on PackageTree itself; we needn't push it into the SourceManager's API.

GetManifestAndLock() is the harder one that may actually need this.

Copy link
Author

Choose a reason for hiding this comment

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

this could just be a method on PackageTree itself;

I actually had that and hit some issues due to the cache. Didn't dig any deeper and just hacked listPackages. You have a better overview for a more elegant implementation.

Copy link
Member

Choose a reason for hiding this comment

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

yep, the cache is not designed to parameterize on a subpath. it'd be quite wasteful to inflate the cache with the additional parameter, and applying it as a post-cache transform wouldn't add much value.

better to have e.g. func (ptree PackageTree) Subtree(subpath string) (PackageTree, error), which produces a new PackageTree containing only the subpackages along that subpath. error could be omitted - OTTOMH the only possible error it could express is if the provided subpath didn't match any packages in the receiver PackageTree.

i think this would fit in quite snugly with the other PackageTree methods.

@sdboyer sdboyer mentioned this pull request Aug 16, 2017
@mvdan
Copy link
Member

mvdan commented Sep 4, 2020

Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks!

@mvdan mvdan closed this Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants