-
Notifications
You must be signed in to change notification settings - Fork 456
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
fix(cli): Better handle non-registry modules and improved naming #929
Conversation
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.
Looks pretty good - the only thing I'm not sure about is, if this would increase the likelihood of name collisions for exports within a module. Other than that 👍
Module outputs are suffixed, so there shouldn't be any collisions there. It is definitely possible to have different modules with the same name; however, I wouldn't imagine that to be likely. It's also pretty easy to explicitly specify different names. |
1b96b76
to
8c7bab0
Compare
8c7bab0
to
0323f11
Compare
Since we're working on docs right now, I believe this impacts this as well. Happy to decouple the docs part from this PR, but we'll have to keep track of this. Other than that, I'm good with these changes. Is this mergeable @jsteinich? |
I had actually missed making any docs changes in this PR. I can either make another PR to update them (if module content is done already) or just make an issue to track making some updates. Should be good to merge. I have a follow up PR for updating the convert command that I'll update after this is merged. |
Let's just go with an issue to track this. The docs are being rewritten anyway. |
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Module sources can take many forms and after doing some more testing I discovered that the module binding generation didn't handle many non-registry sourced modules.
This PR attempts to smartly process the module source string in order to strip out information that isn't relevant / breaks module binding generation. It also tries to assign logical (and generally simpler) naming. Because of these naming changes, it is a breaking change.
It would be possible to not change naming for at least registry sources, but I think having consistency is good and this also returns to (or closer to) the naming that existed for the old registry only module generation.
The general approach taken is as follows:
name
starts as last path part for local & non-registry, 2nd to last part for registry
if submodule, last part of that becomes the name
fqn
full path after relative marker for local, full source for registry, else after things stripped out
submodule separator replace with single to prevent issues with repeated separators
namespace
path part before last part for local & non-registry, {part before name}/{provider} for registry
if submodule, {starting namespace}/{starting name}/{other parts of submodule}
Make name used for class name
Filename combination of namespace and name
Namespace for java package, c# namespace, go namespace, & python module (with appropriate replacements)
fqn for module key
If everyone is good with this approach, I'll go ahead and update integration tests, examples, and documentation to match.
This is intended as a stepping stone to fix #860 by returning to using module bindings rather than hcl module. This PR will cause some issues with convert for registry modules. I'd like to fix in a follow up PR since it will involve some refactoring that will make this one harder to read, but I can also fix with this one.