Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose deprecation data in Bundle #59

Merged
merged 8 commits into from
Jun 10, 2024
72 changes: 56 additions & 16 deletions sourcebundle/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,18 @@ type Builder struct {
// selected version of each module registry package.
resolvedRegistry map[registryPackageVersion]sourceaddrs.RemoteSource

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a code comment

Suggested change
// resolvedRegistryVersionDeprecations tracks potential deprecations for
// each package version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included 👍 ea8f696

// packageVersionDeprecations tracks potential deprecations for
// each package version. If a package version is not deprecated, its mapped value will be nil.
// This data, including both package versions and their potential deprecations, is gathered from the registry client and cached. It is included in the bundle,
// where it can be used to warn about deprecated package versions.
packageVersionDeprecations map[registryPackageVersion]*RegistryVersionDeprecation
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why this is a map of pointer-to-RegistryVersionDeprecation. Later on we store nil in this map if a registry package version has no deprecation information, rather than having no entry in the map at all. Is that important?

Having a pointer here means we need to be diligent about nil checks, or we'll panic. Would there be a disadvantage to this being map[registryPackageVersion]RegistryVersionDeprecation instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from the approach we've taken with how we denote deprecated versions. In essence, when there are no deprecations, the registry api doesn't include it in the Module Version response. So in this waynil denotes an un-deprecated module.


// registryPackageVersions caches responses from module registry calls to
// look up the available versions for a particular module package. Although
// these could potentially change while we're running, we assume that the
// lifetime of a particular Builder is short enough for that not to
// matter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth noting here that this cache includes the deprecation data, as well as the package versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included 👍 be5d907

registryPackageVersions map[regaddr.ModulePackage]versions.List
registryPackageVersions map[regaddr.ModulePackage][]ModulePackageInfo

mu sync.Mutex
}
Expand All @@ -100,14 +106,15 @@ func NewBuilder(targetDir string, fetcher PackageFetcher, registryClient Registr
return nil, fmt.Errorf("invalid target directory: %w", err)
}
return &Builder{
targetDir: absDir,
fetcher: fetcher,
registryClient: registryClient,
analyzed: make(map[remoteArtifact]struct{}),
remotePackageDirs: make(map[sourceaddrs.RemotePackage]string),
remotePackageMeta: make(map[sourceaddrs.RemotePackage]*PackageMeta),
resolvedRegistry: make(map[registryPackageVersion]sourceaddrs.RemoteSource),
registryPackageVersions: make(map[regaddr.ModulePackage]versions.List),
targetDir: absDir,
fetcher: fetcher,
registryClient: registryClient,
analyzed: make(map[remoteArtifact]struct{}),
remotePackageDirs: make(map[sourceaddrs.RemotePackage]string),
remotePackageMeta: make(map[sourceaddrs.RemotePackage]*PackageMeta),
resolvedRegistry: make(map[registryPackageVersion]sourceaddrs.RemoteSource),
packageVersionDeprecations: make(map[registryPackageVersion]*RegistryVersionDeprecation),
registryPackageVersions: make(map[regaddr.ModulePackage][]ModulePackageInfo),
}, nil
}

Expand Down Expand Up @@ -343,7 +350,8 @@ func (b *Builder) findRegistryPackageSource(ctx context.Context, sourceAddr sour
trace := buildTraceFromContext(ctx)

pkgAddr := sourceAddr.Package()
availableVersions, ok := b.registryPackageVersions[pkgAddr]
availablePackageInfos, ok := b.registryPackageVersions[pkgAddr]
var availableVersions versions.List
if !ok {
var reqCtx context.Context
if cb := trace.RegistryPackageVersionsStart; cb != nil {
Expand All @@ -360,14 +368,15 @@ func (b *Builder) findRegistryPackageSource(ctx context.Context, sourceAddr sour
}
return sourceaddrs.RemoteSource{}, fmt.Errorf("failed to query available versions for %s: %w", pkgAddr, err)
}
vs := resp.Versions
vs.Sort()
availableVersions = vs
b.registryPackageVersions[pkgAddr] = availableVersions

availablePackageInfos = resp.Versions
b.registryPackageVersions[pkgAddr] = resp.Versions
availableVersions = extractVersionListFromResponse(availablePackageInfos)
if cb := trace.RegistryPackageVersionsSuccess; cb != nil {
cb(reqCtx, pkgAddr, availableVersions)
}
} else {
availableVersions = extractVersionListFromResponse(availablePackageInfos)
if cb := trace.RegistryPackageVersionsAlready; cb != nil {
cb(ctx, pkgAddr, availableVersions)
}
Expand Down Expand Up @@ -401,6 +410,25 @@ func (b *Builder) findRegistryPackageSource(ctx context.Context, sourceAddr sour
}
realSourceAddr = resp.SourceAddr
b.resolvedRegistry[pkgVer] = realSourceAddr

var versionDeprecation *ModulePackageVersionDeprecation
for _, v := range availablePackageInfos {
if selectedVersion.Same(v.Version) {
versionDeprecation = v.Deprecation
break
}

}
var deprecation *RegistryVersionDeprecation
if versionDeprecation != nil {
deprecation = &RegistryVersionDeprecation{
Version: selectedVersion.String(),
Reason: versionDeprecation.Reason,
Link: versionDeprecation.Link,
}
}
b.packageVersionDeprecations[pkgVer] = deprecation

if cb := trace.RegistryPackageSourceSuccess; cb != nil {
cb(reqCtx, pkgAddr, selectedVersion, realSourceAddr)
}
Expand Down Expand Up @@ -576,7 +604,7 @@ func (b *Builder) writeManifest(filename string) error {
})

registryObjs := make(map[regaddr.ModulePackage]*manifestRegistryMeta)
for rpv, sourceAddr := range b.resolvedRegistry {
for rpv, sourceInfo := range b.resolvedRegistry {
manifestMeta, ok := registryObjs[rpv.pkg]
if !ok {
root.RegistryMeta = append(root.RegistryMeta, manifestRegistryMeta{
Expand All @@ -586,10 +614,13 @@ func (b *Builder) writeManifest(filename string) error {
manifestMeta = &root.RegistryMeta[len(root.RegistryMeta)-1]
registryObjs[rpv.pkg] = manifestMeta
}
deprecation := b.packageVersionDeprecations[rpv]
manifestMeta.Versions[rpv.version.String()] = manifestRegistryVersion{
SourceAddr: sourceAddr.String(),
SourceAddr: sourceInfo.String(),
Deprecation: deprecation,
}
}

sort.Slice(root.RegistryMeta, func(i, j int) bool {
return root.Packages[i].SourceAddr < root.Packages[j].SourceAddr
})
Expand Down Expand Up @@ -713,3 +744,12 @@ func packagePrepareWalkFn(root string, ignoreRules *ignorefiles.Ruleset) filepat
return nil
}
}

func extractVersionListFromResponse(modPackageInfos []ModulePackageInfo) versions.List {
vs := make(versions.List, len(modPackageInfos))
for index, v := range modPackageInfos {
vs[index] = v.Version
}
vs.Sort()
return vs
}
119 changes: 116 additions & 3 deletions sourcebundle/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func TestBuilderSimple(t *testing.T) {
"1.0.0": "https://example.com/foo.tgz",
},
},
nil,
)

realSource := sourceaddrs.MustParseSource("https://example.com/foo.tgz").(sourceaddrs.RemoteSource)
Expand Down Expand Up @@ -114,6 +115,7 @@ func TestBuilderSubdirs(t *testing.T) {
"1.0.0": "https://example.com/subdirs.tgz//a",
},
},
nil,
)

// NOTE: We're asking for subdir "b" of the registry address. That combines
Expand Down Expand Up @@ -183,6 +185,7 @@ func TestBuilderRemoteDeps(t *testing.T) {
"https://example.com/dependency2.tgz": "testdata/pkgs/terraformignore",
},
nil,
nil,
)

startSource := sourceaddrs.MustParseSource("https://example.com/with-deps.tgz").(sourceaddrs.RemoteSource)
Expand Down Expand Up @@ -269,6 +272,92 @@ func TestBuilderRemoteDeps(t *testing.T) {
})
}

func TestBuilderRegistryVersionDeprecation(t *testing.T) {
// This tests the common pattern of specifying a module registry address
// to start, having that translated into a real remote source address,
// and then downloading from that real source address. There are no
// oddities or edge-cases here.

tracer := testBuildTracer{}
ctx := tracer.OnContext(context.Background())

targetDir := t.TempDir()
builder := testingBuilder(
t, targetDir,
map[string]string{
"https://example.com/foo.tgz": "testdata/pkgs/hello",
},
map[string]map[string]string{
"example.com/foo/bar/baz": {
"1.0.0": "https://example.com/foo.tgz",
},
},
map[string]map[string]*ModulePackageVersionDeprecation{
"example.com/foo/bar/baz": {
"1.0.0": &ModulePackageVersionDeprecation{
Reason: "test reason",
Link: "test link",
},
},
},
)

realSource := sourceaddrs.MustParseSource("https://example.com/foo.tgz").(sourceaddrs.RemoteSource)
regSource := sourceaddrs.MustParseSource("example.com/foo/bar/baz").(sourceaddrs.RegistrySource)
diags := builder.AddRegistrySource(ctx, regSource, versions.All, noDependencyFinder)
if len(diags) > 0 {
t.Fatal("unexpected diagnostics")
}

bundle, err := builder.Close()

if err != nil {
t.Fatalf("failed to close bundle: %s", err)
}

version, _ := versions.ParseVersion("1.0.0")
pkgAddr, _ := sourceaddrs.ParseRegistryPackage("example.com/foo/bar/baz")

wantDeprecations := map[regaddr.ModulePackage]map[versions.Version]*RegistryVersionDeprecation{
pkgAddr: {
version: &RegistryVersionDeprecation{
Version: "1.0.0",
Reason: "test reason",
Link: "test link",
},
},
}
gotDeprecations := bundle.registryPackageVersionDeprecations
if diff := cmp.Diff(wantDeprecations, gotDeprecations); diff != "" {
t.Errorf("wrong deprecations\n%s", diff)
}

localPkgDir, err := bundle.LocalPathForRemoteSource(realSource)
if err != nil {
for pkgAddr, localDir := range builder.remotePackageDirs {
t.Logf("contents of %s are in %s", pkgAddr, localDir)
}
t.Fatalf("builder does not know a local directory for %s: %s", realSource.Package(), err)
}

if info, err := os.Lstat(filepath.Join(localPkgDir, "hello")); err != nil {
t.Errorf("problem with output file: %s", err)
} else if !info.Mode().IsRegular() {
t.Errorf("output file is not a regular file")
}

// Looking up the original registry address at the selected version
// should return the same directory, because the registry address is just
// an indirection over the same source address.
registryPkgDir, err := bundle.LocalPathForRegistrySource(regSource, versions.MustParseVersion("1.0.0"))
if err != nil {
t.Fatalf("builder does not know a local directory for %s: %s", regSource.Package(), err)
}
if registryPkgDir != localPkgDir {
t.Errorf("local dir for %s doesn't match local dir for %s", regSource, realSource)
}
}

func TestBuilderRemoteDepsDifferingTypes(t *testing.T) {
tracer := testBuildTracer{}
ctx := tracer.OnContext(context.Background())
Expand All @@ -282,6 +371,7 @@ func TestBuilderRemoteDepsDifferingTypes(t *testing.T) {
"https://example.com/dependency2.tgz": "testdata/pkgs/terraformignore",
},
nil,
nil,
)

startSource := sourceaddrs.MustParseSource("https://example.com/self_dependency.tgz").(sourceaddrs.RemoteSource)
Expand Down Expand Up @@ -383,6 +473,7 @@ func TestBuilderTerraformIgnore(t *testing.T) {
"https://example.com/ignore.tgz": "testdata/pkgs/terraformignore",
},
nil,
nil,
)

startSource := sourceaddrs.MustParseSource("https://example.com/ignore.tgz").(sourceaddrs.RemoteSource)
Expand Down Expand Up @@ -442,6 +533,7 @@ func TestBuilderCoalescePackages(t *testing.T) {
"https://example.com/dependency2.tgz": "testdata/pkgs/hello",
},
nil,
nil,
)

startSource := sourceaddrs.MustParseSource("https://example.com/with-deps.tgz").(sourceaddrs.RemoteSource)
Expand Down Expand Up @@ -542,7 +634,7 @@ func TestBuilderCoalescePackages(t *testing.T) {
})
}

func testingBuilder(t *testing.T, targetDir string, remotePackages map[string]string, registryPackages map[string]map[string]string) *Builder {
func testingBuilder(t *testing.T, targetDir string, remotePackages map[string]string, registryPackages map[string]map[string]string, registryVersionDeprecations map[string]map[string]*ModulePackageVersionDeprecation) *Builder {
t.Helper()

type fakeRemotePackage struct {
Expand All @@ -557,6 +649,7 @@ func testingBuilder(t *testing.T, targetDir string, remotePackages map[string]st

remotePkgs := make([]fakeRemotePackage, 0, len(remotePackages))
registryPkgs := make([]fakeRegistryPackage, 0, len(registryPackages))
registryDeprecations := make(map[string]map[versions.Version]*ModulePackageVersionDeprecation)

for pkgAddrRaw, localDir := range remotePackages {
pkgAddr, err := sourceaddrs.ParseRemotePackage(pkgAddrRaw)
Expand Down Expand Up @@ -593,6 +686,23 @@ func testingBuilder(t *testing.T, targetDir string, remotePackages map[string]st
registryPkgs = append(registryPkgs, pkg)
}

for pkgAddrRaw, deprecations := range registryVersionDeprecations {
pkgAddr, err := sourceaddrs.ParseRegistryPackage(pkgAddrRaw)
if err != nil {
t.Fatalf("invalid registry package address %q: %s", pkgAddrRaw, err)
}

for versionRaw, versionDeprecation := range deprecations {
version, err := versions.ParseVersion(versionRaw)
if err != nil {
t.Fatalf("invalid registry package version %q for %s: %s", versionRaw, pkgAddr, err)
}
registryDeprecations[pkgAddr.Namespace] = map[versions.Version]*ModulePackageVersionDeprecation{
version: versionDeprecation,
}
}
}

fetcher := packageFetcherFunc(func(ctx context.Context, sourceType string, url *url.URL, targetDir string) (FetchSourcePackageResponse, error) {
var ret FetchSourcePackageResponse
// Our fake implementation of "fetching" is to just copy one local
Expand Down Expand Up @@ -621,9 +731,12 @@ func testingBuilder(t *testing.T, targetDir string, remotePackages map[string]st
if pkg.pkgAddr != pkgAddr {
continue
}
ret.Versions = make(versions.List, len(pkg.versions))
ret.Versions = make([]ModulePackageInfo, len(pkg.versions))
for version := range pkg.versions {
ret.Versions = append(ret.Versions, version)
ret.Versions = append(ret.Versions, ModulePackageInfo{
Version: version,
Deprecation: registryDeprecations[pkg.pkgAddr.Namespace][version],
})
}
return ret, nil
}
Expand Down
22 changes: 17 additions & 5 deletions sourcebundle/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ type Bundle struct {
remotePackageDirs map[sourceaddrs.RemotePackage]string
remotePackageMeta map[sourceaddrs.RemotePackage]*PackageMeta

registryPackageSources map[regaddr.ModulePackage]map[versions.Version]sourceaddrs.RemoteSource
registryPackageSources map[regaddr.ModulePackage]map[versions.Version]sourceaddrs.RemoteSource
registryPackageVersionDeprecations map[regaddr.ModulePackage]map[versions.Version]*RegistryVersionDeprecation
}

// OpenDir opens a bundle rooted at the given base directory.
Expand All @@ -51,10 +52,11 @@ func OpenDir(baseDir string) (*Bundle, error) {
}

ret := &Bundle{
rootDir: rootDir,
remotePackageDirs: make(map[sourceaddrs.RemotePackage]string),
remotePackageMeta: make(map[sourceaddrs.RemotePackage]*PackageMeta),
registryPackageSources: make(map[regaddr.ModulePackage]map[versions.Version]sourceaddrs.RemoteSource),
rootDir: rootDir,
remotePackageDirs: make(map[sourceaddrs.RemotePackage]string),
remotePackageMeta: make(map[sourceaddrs.RemotePackage]*PackageMeta),
registryPackageSources: make(map[regaddr.ModulePackage]map[versions.Version]sourceaddrs.RemoteSource),
registryPackageVersionDeprecations: make(map[regaddr.ModulePackage]map[versions.Version]*RegistryVersionDeprecation),
}

manifestSrc, err := os.ReadFile(filepath.Join(rootDir, manifestFilename))
Expand Down Expand Up @@ -108,11 +110,17 @@ func OpenDir(baseDir string) (*Bundle, error) {
vs = make(map[versions.Version]sourceaddrs.RemoteSource)
ret.registryPackageSources[pkgAddr] = vs
}
deprecations := ret.registryPackageVersionDeprecations[pkgAddr]
if deprecations == nil {
deprecations = make(map[versions.Version]*RegistryVersionDeprecation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary reallocation I think (see previous comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that similar to the above we could just assign the nested map at the index of pkgAddr but I figured to was best to stick with the style of how registryPackageSources is handled above for consistencies sake.

vs := ret.registryPackageSources[pkgAddr]
if vs == nil {
    vs = make(map[versions.Version]sourceaddrs.RemoteSource)
    ret.registryPackageSources[pkgAddr] = vs
}

ret.registryPackageVersionDeprecations[pkgAddr] = deprecations
}
for versionStr, mv := range rpm.Versions {
version, err := versions.ParseVersion(versionStr)
if err != nil {
return nil, fmt.Errorf("invalid registry package version %q: %w", versionStr, err)
}
deprecations[version] = mv.Deprecation
sourceAddr, err := sourceaddrs.ParseRemoteSource(mv.SourceAddr)
if err != nil {
return nil, fmt.Errorf("invalid registry package source address %q: %w", mv.SourceAddr, err)
Expand Down Expand Up @@ -354,6 +362,10 @@ func (b *Bundle) RegistryPackageVersions(pkgAddr regaddr.ModulePackage) versions
return ret
}

func (b *Bundle) RegistryPackageVersionDeprecation(pkgAddr regaddr.ModulePackage, version versions.Version) *RegistryVersionDeprecation {
return b.registryPackageVersionDeprecations[pkgAddr][version]
}

// RegistryPackageSourceAddr returns the remote source address corresponding
// to the given version of the given module package, or sets its second return
// value to false if no such version is included in the bundle.
Expand Down
3 changes: 2 additions & 1 deletion sourcebundle/manifest_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ type manifestRegistryVersion struct {
// This SourceAddr is a full source address, so it might potentially
// have a sub-path portion. If it does then it must be combined with
// any sub-path included in the user's registry module source address.
SourceAddr string `json:"source"`
SourceAddr string `json:"source"`
Deprecation *RegistryVersionDeprecation `json:"deprecation"`
}

type manifestPackageMeta struct {
Expand Down
Loading