Expose deprecation data in Bundle #59

Maed223 · 2024-05-14T15:10:41Z

Updates sourceBundle’s ModulePackageVersionsResponse type to include registry module deprecation data in the response. This deprecation data is then propagated to the Bundle, where it will be then used within a related tfc-agent PR to add diagnostic warnings about deprecated module versions for stacks .

Related Items

…include it in the manifest

brandonc

Got a couple concerns with this, but they are minor. We should find a way to create a meaningful test

sourcebundle/builder.go

sourceaddrs/source_remote.go

…le, use index to append elements to slice with known length and capacity

brandonc · 2024-06-06T17:29:17Z

sourceaddrs/source_registry.go

@@ -29,6 +29,12 @@ type RegistrySource struct {
 	subPath string
 }

+type RegistryVersionDeprecation struct {


This type may be in the wrong package, too.

I like the way you un-nested the thing previously called "RemoteSourceInfo"!

Ah gotcha. Just to clarify your thinking, would you consider it out of place since it's only used in sourcebundle?

And in any case any suggestions where it might be a better fit? No file in particular pops out at me in the package.

Ended up moving it to package_meta.go in the sourcebundle package, seems to be the best fit.

Included here: 3914531

uturunku1

Thanks for walking me through a demo of the changes. To recap, you'll adjust the "more information" section to include a url that can actually be clicked.
The changes we smoked tested today were through the agent, which is good. But I'd also recommend that you smoke test by building the binary for stacks, running some of their commands in the CLI, and just confirming that nothing is broken over there.
I took a look at your code changes yesterday, and again today. I really like that after you addressed Brandon's comment, the scope of your changes has reduced and simplified, in a good way!
While I am absent, please, continue with Brandon, and if possible with someone from the Stacks project to get their blessing on these changes 🙏

brandonc

Really basic feedback. I didn't smoke test but looks safe

brandonc · 2024-06-07T16:59:16Z

sourcebundle/builder.go

@@ -73,12 +73,14 @@ type Builder struct {
 	// selected version of each module registry package.
 	resolvedRegistry map[registryPackageVersion]sourceaddrs.RemoteSource



Add a code comment

Suggested change

// resolvedRegistryVersionDeprecations tracks potential deprecations for

// each package version

Included 👍 ea8f696

brandonc · 2024-06-07T17:01:10Z

sourcebundle/builder.go

+		remotePackageDirs:                   make(map[sourceaddrs.RemotePackage]string),
+		remotePackageMeta:                   make(map[sourceaddrs.RemotePackage]*PackageMeta),
+		resolvedRegistry:                    make(map[registryPackageVersion]sourceaddrs.RemoteSource),
+		resolvedRegistryVersionDeprecations: make(map[registryPackageVersion]*RegistryVersionDeprecation),


Slightly verbose-- plus 'resolved' may refer to the RemoteSource values...

Suggested change

resolvedRegistryVersionDeprecations: make(map[registryPackageVersion]*RegistryVersionDeprecation),

versionDeprecations: make(map[registryPackageVersion]*RegistryVersionDeprecation),

Could we meet in the middle with packageVersionDeprecations?

Included here: ea8f696

brandonc · 2024-06-07T17:18:58Z

sourcebundle/bundle.go

+	deprecation := b.registryPackageVersionDeprecations[pkgAddr][version]
+	return deprecation


I thought this could panic, but it wont! Neat. No need for a local, though

Suggested change

deprecation := b.registryPackageVersionDeprecations[pkgAddr][version]

return deprecation

return b.registryPackageVersionDeprecations[pkgAddr][version]

Included 👍 ea8f696

brandonc · 2024-06-07T17:38:05Z

sourcebundle/builder_test.go

+		if registryDeprecations[pkgAddr.Namespace] == nil {
+			registryDeprecations[pkgAddr.Namespace] = make(map[versions.Version]*ModulePackageVersionDeprecation)
+		}


I don't think you need to reallocate this nested map: https://go.dev/play/p/dmUQYnvF7y_H

That's what I thought initially as well, but I'm otherwise getting nil pointer exceptions. Any idea why that could be the case?

Actually nevermind that 😅, didn't initially catch that you were assigning a map at the index.

Included 👍 ea8f696

brandonc · 2024-06-07T17:38:57Z

sourcebundle/bundle.go

@@ -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)


Unnecessary reallocation I think (see previous comment)

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 }

…s, remove unneed map reallocation

alisdair

Just some minor go-specific thoughts. From a stacks technical point of view, this looks fine to me!

alisdair · 2024-06-10T18:24:16Z

sourcebundle/builder.go

@@ -73,12 +73,16 @@ type Builder struct {
 	// selected version of each module registry package.
 	resolvedRegistry map[registryPackageVersion]sourceaddrs.RemoteSource

+	// resolvedRegistryVersionDeprecations tracks potential deprecations for
+	// each package version
+	packageVersionDeprecations map[registryPackageVersion]*RegistryVersionDeprecation


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?

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.

alisdair · 2024-06-10T18:29:52Z

sourcebundle/builder.go

@@ -73,12 +73,16 @@ type Builder struct {
 	// selected version of each module registry package.
 	resolvedRegistry map[registryPackageVersion]sourceaddrs.RemoteSource

+	// resolvedRegistryVersionDeprecations tracks potential deprecations for
+	// each package version


This comment is currently just restating the variable name, and some more detail would be helpful. Example future readers' questions you may want to proactively answer (or not! whatever you think is pertinent):

Where is the data gathered from?

What consumes this data?

What's a "potential" deprecation?

While we're here, the field in the comment doesn't match the field name in the struct.

Included 👍 be5d907

alisdair · 2024-06-10T18:33:57Z

sourcebundle/builder.go

+	// resolvedRegistryVersionDeprecations tracks potential deprecations for
+	// each package version
+	packageVersionDeprecations map[registryPackageVersion]*RegistryVersionDeprecation
+
 	// 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.


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

Included 👍 be5d907

alisdair · 2024-06-10T18:36:23Z

sourcebundle/builder.go

+	return vs
+}
+
+func extractVersionDeprecationsFromResponse(modPackageInfos []ModulePackageInfo) map[versions.Version]*ModulePackageVersionDeprecation {


This function is very simple and only called from one site. I think it's more typical go style to inline this instead.

Included 👍 be5d907

Maed223 added 3 commits June 4, 2024 19:28

Expose module deprecation data from the registry within the builder, …

6d6b2e4

…include it in the manifest

Include module version deprecation data in bundle

bb038d6

test fix

ef8c8ac

Maed223 force-pushed the TF-12458/expose-deprecation-attributes branch from b8e6fc8 to ef8c8ac Compare June 4, 2024 23:29

Maed223 marked this pull request as ready for review June 5, 2024 00:09

Maed223 changed the title ~~Expose deprecation data in ModulePackageVersionsResponse~~ Expose deprecation data in Bundle Jun 5, 2024

brandonc reviewed Jun 5, 2024

View reviewed changes

sourcebundle/builder.go Outdated Show resolved Hide resolved

sourceaddrs/source_remote.go Outdated Show resolved Hide resolved

feedback from @brandonc: move deprecation related types to sourcebund…

3fa6575

…le, use index to append elements to slice with known length and capacity

brandonc reviewed Jun 6, 2024

View reviewed changes

Maed223 added 2 commits June 6, 2024 13:43

added test

e1d3105

move RegistryVersionDeprecation to sourcebundle package

3914531

uturunku1 reviewed Jun 6, 2024

View reviewed changes

brandonc reviewed Jun 7, 2024

View reviewed changes

Updated with feedback from @brandonc: comments, improve variable name…

ea8f696

…s, remove unneed map reallocation

alisdair approved these changes Jun 10, 2024

View reviewed changes

Updated with feedback from @alisdair

be5d907

Maed223 merged commit d050c34 into main Jun 10, 2024
9 checks passed

Maed223 deleted the TF-12458/expose-deprecation-attributes branch June 10, 2024 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose deprecation data in Bundle #59

Expose deprecation data in Bundle #59

Maed223 commented May 14, 2024 •

edited

Loading

brandonc left a comment

brandonc Jun 6, 2024

brandonc Jun 6, 2024

Maed223 Jun 6, 2024

Maed223 Jun 6, 2024

Maed223 Jun 6, 2024 •

edited

Loading

uturunku1 left a comment

brandonc left a comment

brandonc Jun 7, 2024

Maed223 Jun 7, 2024

brandonc Jun 7, 2024

Maed223 Jun 7, 2024

brandonc Jun 7, 2024

Maed223 Jun 7, 2024

brandonc Jun 7, 2024

Maed223 Jun 7, 2024

Maed223 Jun 7, 2024

Maed223 Jun 7, 2024

brandonc Jun 7, 2024

Maed223 Jun 7, 2024

alisdair left a comment

alisdair Jun 10, 2024

Maed223 Jun 10, 2024

alisdair Jun 10, 2024

Maed223 Jun 10, 2024

alisdair Jun 10, 2024

Maed223 Jun 10, 2024

alisdair Jun 10, 2024

Maed223 Jun 10, 2024

		@@ -73,12 +73,14 @@ type Builder struct {
		// selected version of each module registry package.
		resolvedRegistry map[registryPackageVersion]sourceaddrs.RemoteSource



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

	resolvedRegistryVersionDeprecations: make(map[registryPackageVersion]*RegistryVersionDeprecation),
	versionDeprecations: make(map[registryPackageVersion]*RegistryVersionDeprecation),

		deprecation := b.registryPackageVersionDeprecations[pkgAddr][version]
		return deprecation

	deprecation := b.registryPackageVersionDeprecations[pkgAddr][version]
	return deprecation
	return b.registryPackageVersionDeprecations[pkgAddr][version]

Expose deprecation data in Bundle #59

Expose deprecation data in Bundle #59

Conversation

Maed223 commented May 14, 2024 • edited Loading

Related Items

brandonc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maed223 Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

uturunku1 left a comment

Choose a reason for hiding this comment

brandonc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisdair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maed223 commented May 14, 2024 •

edited

Loading

Maed223 Jun 6, 2024 •

edited

Loading