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

Improve: TF users #29

Closed
hacklschorsch opened this issue Jan 27, 2025 · 1 comment · Fixed by #36
Closed

Improve: TF users #29

hacklschorsch opened this issue Jan 27, 2025 · 1 comment · Fixed by #36
Labels
enhancement New feature or request

Comments

@hacklschorsch
Copy link
Member

hacklschorsch commented Jan 27, 2025

The code in this repository should be of exemplary quality.

Let's start with something simple.

https://github.com/tahoe-lafs/infrastructure/blob/67392a2a35512ffd175d05448484b280c8791f77/tf/core/users.tf sports 44 lines to define two users.

I am asking myself: If all it does is to define two users, why does it have more than two lines? Or four lines? It has 22 x as many lines. Why?

It also includes two undocumented utility functions with no mention why we have them or what they're good for.

Let's clean this file up!

@hacklschorsch hacklschorsch added the enhancement New feature or request label Jan 27, 2025
@btlogy
Copy link
Member

btlogy commented Jan 29, 2025

I think @hacklschorsch is partly correct: this complexity is not currently required (as explained in the header of the file).

If the content of the file is confusing, the least I can do is to add some comments (adding more "LoC" though).

users.tf sports 44 lines

Maybe we could go down to 30 lines by removing the comments and compacting some expressions w/o changing the code. Though, I bet the fmt check will prevent some of this.

why does it have more than two lines? Or four lines?

User's definition usually requires an identifier (1 line) AND some attributes like an ssh key (more lines).

Or four lines?

OpenToFu works with resources we need to call by name and these requires 2 additional lines to wrap around those attributes.

... to define two users.
It has 22 x as many lines. Why?

Partly explained above and also because the resources we create here at the end are NOT the users (not yet, hence the first comment: can be re-used elsewhere).
Those resources are of the type hcloud_ssh_key and a single user can have more than one.

It also includes two undocumented utility functions ...

If we are referring to flatten and format, I do not agree with "undocumented" qualifier:

... with no mention why we have them or what they're good for.

Not entirely true! There is one comment:

# Flatten all the ssh keys of each users

Which surely can be improved: the goal is to collect all ssh keys from all the users (hence flatten) and use format to create a unique identifier for each keys.

I will improve the comments first and leave the possible removal of the flexibility it offers (e.g.: re-use of the user's data for other resource or rotation of the ssh keys) for later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants