Fixed a variety of issues #5
Conversation
|
Hey @marcusmarlowe0x , Thanks for the contribution. Why are you using it to generate resource blocks? It's designed to be used with terraform plan --generate-config-out. Surely the resources it makes aren't valid because they only have a name? Also, I'm not supportive of the CLI main.py addition. It goes against the intended use for the project and adds additional maintenance requirements. I have skimmed the rest of it and will produce a more formal review later, but wanted to call this out first. Cheers |
There was a problem hiding this comment.
I appreciate totally that this repo gets sporadic attention and is not in a fit for purpose state, but it will be completed as and when we need it.
This PR goes against the intended use case of this library. Whilst the contribution is appreciated, I won't be merging it in it's current state and request a rethink of the changes proposed and their value.
To summarise my comments:
-
This is not a CLI tool and proposed changes which push it in that direction will be rejected. If a CLI tool was to be developed it would be separate to this library and take advange of it.
-
There is a little bit of an LLM feel to some of the changes, especially with dotted comments, please remove these comments in future contributions. Comments should say why not what!
-
There is no PR comment explaining the reasonsing behind the changes, nor commit history which allow me to follow the thought process.
-
Some things seem illogical - .tf files in the gitignore, config files living in the library. I've highlighted these individually.
Please review the comments I've made.
If there are any proposed changes you'd like to see - or if it flat out doesn't work (which could well be the case at the moment) please raise an issue and I'd be happy to make changes.
Thanks!
J
|
|
||
| return "\n".join(block for block in self.hcl_d().values()) | ||
|
|
||
| blocks = set() # Use a set to deduplicate |
There was a problem hiding this comment.
What potential for duplicates are there? The blocks are populated from the Jamf Pro IDs which are unique per resource?
| for resource_type, hcl in self.hcl_d().items(): | ||
| if hcl: # Only add if there's content | ||
| blocks.add(hcl) | ||
| return "\n".join(sorted(blocks)) # Sort for consistent output |
There was a problem hiding this comment.
What about the output is not consistent?
There was a problem hiding this comment.
This seems to have been made without the prior context of the shape of the data.
| out.setdefault(resource.resource_type, "") | ||
| out[resource.resource_type] += f"\n{hcl}" | ||
| hcl = resource.build_hcl() | ||
| if hcl: # Only add if there's content |
There was a problem hiding this comment.
This is duplicated here and on line 42. hcl_s relies on hcl_d.
|
|
||
| self.targetted = targetted | ||
|
|
||
| self.refresh() |
There was a problem hiding this comment.
This was purposefully omitted to allow control of when API calls begin.
|
|
||
| # Debug the response structure | ||
| response_data = resp.json() | ||
| self.lg.info("API Response: %s", response_data) |
There was a problem hiding this comment.
Don't log large json blocks as INFO.
There was a problem hiding this comment.
Shouldn't be in a library or a CLI tool?
| """Custom exceptions for the jamftf package.""" | ||
|
|
||
| class ConfigError(Exception): | ||
| """Raised when there is an issue with the configuration.""" |
There was a problem hiding this comment.
This error message is less descriptive than the one it replaced.
| """Generate a descriptive name for a resource.""" | ||
|
|
||
| the_resource_name = ''.join(c if c.isalpha() else '_' for c in resource_name) | ||
| # Convert to camelCase |
There was a problem hiding this comment.
There is no conversion to camel case occuring here
| f" to = {resource_type.value}.{resource_type.value}-{jpro_id}\n" | ||
| f"}}\n" | ||
| ) | ||
| def resource_block(resource_type: str, resource_name: str, jpro_id: str) -> str: |
There was a problem hiding this comment.
This library is intended to be used in conjunction with terraform plan --generate-config-out which generates resource blocks. I do not see the requirement to make them here, they'd be invalid too as only a name field is populated.
| """ | ||
|
|
||
| return [import_block(i.resource_type, i.jpro_id) for i in resources] | ||
| def generate_imports(items: list[SingleItem]) -> str: |
There was a problem hiding this comment.
This would generate an alternating structure of hcl blocks which is hard to clean up. Import blocks are only required for 1 run and then can be deleted. Structuring like this would make things tricky.
I misunderstood the purpose of the library. The README.md file gave instructions that were not usable and it wasn't clear that this was a library to be used elsewhere. The top level description did not indicate it was a library and it generally seemed to be in an unusable state. Is this intended to be used by https://github.com/deploymenttheory/terraform-importer-jamfpro? If so can you adjust the README to reflect the intended usage? |
|
It's an unfinished library which served as a showcase for what is possible in terms of automatic generation and migration. The readme was written by AI - as per it's latest commit - and just outlined how to run the code. It will be utilised inside the importer repo you've linked at some point but I can't comment on when due to my commitments internally. I'm happy to implement functional changes as and when I can - feel free to submit issues or another PR for rewiew taking into account the annotations above. Thanks! |
This makes the repository functional