- Notifications
You must be signed in to change notification settings - Fork1k
chore: track terraform module source type in telemetry#15590
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
| ModuleSourceTypeUnknownModuleSourceType="unknown" | ||
| ) | ||
| funcGetModuleSourceType(sourcestring)ModuleSourceType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It might be good for future readers to add a doc comment to this function along with a link tohttps://developer.hashicorp.com/terraform/language/modules/sources, as they may have some similar questions to me before reading:
- Wait, these aren't just URLS?! Nope.
- There's difference between how relative and absolute local paths are handled? yup
- Wait, you can have "localterraform.com" as a generic "local" hostname? yup
- Wait, you can just use an un-prefixed GitHub registry as a module source? yup
- etc...
coderd/telemetry/telemetry.go Outdated
| if!strings.Contains(source,"://")&&!strings.Contains(source,".")&&strings.Contains(source,"/") { | ||
| returnModuleSourceTypePublicRegistry | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we tighten this condition up a bit and assert that it follows the formnamespace/name/provider? And maybe move it up slightly?
@johnstcn I made changes according to your feedback. Thanks! |
| ifstrings.Contains(source,"registry.terraform.io") { | ||
| returnModuleSourceTypePublicRegistry | ||
| } | ||
| ifstrings.Contains(source,"app.terraform.io")||strings.Contains(source,"localterraform.com") { | ||
| returnModuleSourceTypePrivateRegistry | ||
| } | ||
| ifstrings.Contains(source,"registry.coder.com") { | ||
| returnModuleSourceTypeCoderRegistry | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What aboutregistry.coder.com.mydomain.foo.bar? Should we instead usestrings.HasPrefix here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@johnstcn I wasn't aware that people could access the coder registry that way. How does this work? Can people self-host our registry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As I write this, I realise that prefix-checking won't even help here unless we attempt to parse the module as an URL and extract only the hostname.
In any case, I don't see any reason that someone sufficiently motiviated couldn't do this if they controlled the domainmydomain.foo.bar and used something like outsiders/citizen to self-host a registry.
It's more of a contrived hypothetical though and I'm not going to block on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's acceptable to misclassify a small percentage of modules. With telemetry, we’re more focused on aggregates, and cases like this are unlikely to affect the overall picture.
| require.Equal(t,modules[1].Version,"1.0.0") | ||
| require.Equal(t,modules[1].SourceType,telemetry.ModuleSourceTypeCoderRegistry) | ||
| }) | ||
| t.Run("ModuleSourceType",func(t*testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
praise: nice test coverage!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One last comment but I don't need to re-review.
97ce44a intomainUh oh!
There was an error while loading.Please reload this page.
Addresseshttps://github.com/coder/nexus/issues/141