MatthewJohn / terrareg

Open source Terraform module registry with UI, optional Git integration and deep analysis
https://gitlab.dockstudios.co.uk/pub/terrareg
GNU General Public License v3.0
268 stars 20 forks source link

Feature Request: Update Module Version Archive to Include Only Specified Path Contents #41

Closed Klyde-Moradeyo closed 2 months ago

Klyde-Moradeyo commented 6 months ago

Hey,

I've noticed that currently when specifying a git path for creating a module, the archive generated includes the entire repository's contents, not just the specified path.

Current Behavior

When specifying a git path for a module, the archive created by Terrareg contains the whole repository's contents. For example, with repo Structure:

.
├── Dockerfile
├── README.md
└── my-module
    ├── main.tf

If the specified git path is /my-module, Terrareg currently generates an archive reflecting the entire folder structure as shown above.

Proposed Change

Update the module version archive creation process to only include the contents of the specified path. For example if the specified git path is /my-module, the archive would contain:

└── my-module
    ├── main.tf

Benefits

MatthewJohn commented 6 months ago

Hey @Klyde-Moradeyo,

I do agree this makes sense - for compatibility, we'd need to be careful - there's nothing stopping a user providing a git path, having the root of their terraform in a sub-module, but have the terraform reference files (or even other modules) in the parent folders. One core thing to keep in mind is that, currently, the archive response v.s. a git URL response is the same - Terraform obtains the whole archive, similar to how it clones the entire repository. I'd be happy to introduce this as an optional functionality, though. Whether this would be best served as a global configuration to "only return data from a git_path sub-directory" or whether it be a per-module configuration - happy for any input on this.

Thanks Matt

MatthewJohn commented 6 months ago

Created gitlab issue: https://gitlab.dockstudios.co.uk/pub/terrareg/-/issues/513 gitlab-issue-id:513

Klyde-Moradeyo commented 6 months ago

Thanks for the quick response. With those compatibility issues you mentioned, I agree that the optional feature is the best path forward. I think a per-module configuration would be good as it would offer the more granularity by allowing each module to operate based on its specific needs and context.

MatthewJohn commented 6 months ago

Hi @Klyde-Moradeyo,

I started implementing this, when I realised I'd forgotten quite an important fact:

The archives are generated in both cases (mainly for future features to allow archive downloading of git-based modules - but this hasn't been prioritised yet - https://gitlab.dockstudios.co.uk/pub/terrareg/-/issues/490). The generation of these archives can be avoided by setting https://matthewjohn.github.io/terrareg/CONFIG/#delete_externally_hosted_artifacts

This can be verified by:

Could you verify your use-case and how you'd like to handle these modules :)

Matt

Klyde-Moradeyo commented 6 months ago

Ah, I understand now. I hadn't realized that users accessing the module were directly cloning from the Git repository instead of using the archive. This seems like it could be a good addition onto issue #490

My usecase is that I’m looking to hide parts of our monorepo serving different terrareg instances from multiple users for role-based access.

MatthewJohn commented 6 months ago

This seems like it could be a good addition onto issue #490

Agreed :)

If this is something that would be useful, I can prioritise these two and try to get implemented#40

Matt

Klyde-Moradeyo commented 6 months ago

That will be great! Thanks @MatthewJohn

MatthewJohn commented 5 months ago

The implementation of enforcing archive downloads is now done - released in v3.3.0 I'm trying to get another release out and will then finish tests and such for the remainder of this task :)

Matt

Klyde-Moradeyo commented 5 months ago

Thanks Matt! I'm currently trying to get this work but I can't find any flags on the UI for this. Could you give directions?

MatthewJohn commented 5 months ago

Apologies, as I say, the first release was just the enforcing artifacts, not to be able to limit by path yet. Will try to find some time for this this week :)

Klyde-Moradeyo commented 5 months ago

Hey @MatthewJohn, just a headsup - I've decided to use the source archive feature instead for serving my modules so this isn't urgent for me anymore

MatthewJohn commented 5 months ago

Hey @Klyde-Moradeyo,

That's great to hear - sorry, it's been quite turmoil here and difficult to find time! Would the feature be useful enough to switch back to it in the future?

If not, I can park the work until someone shows interest in it in future :)

Matt

Klyde-Moradeyo commented 5 months ago

That's great to hear - sorry, it's been quite turmoil here and difficult to find time! Would the feature be useful enough to switch back to it in the future?

No worries, I understand its hard to find time for these things. In my case I doubt I'll revert as I've built automation around the source archive feature to address my initial problem statement.

MatthewJohn commented 2 months ago

I'm about ready to merge this - just trying to fix some flaky tests (generally flaky ones, not specific to this)

MatthewJohn commented 2 months ago

Released in v3.8.0