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

export command #54

Merged
merged 87 commits into from
Mar 16, 2025
Merged

export command #54

merged 87 commits into from
Mar 16, 2025

Conversation

daveads
Copy link
Contributor

@daveads daveads commented Dec 17, 2024

feat: add terraform environment export command

  • Add new permit env export terraform command
  • Support exporting environment configuration to HCL format
  • Add optional API key and output file path parameters
  • Export resources, roles, user sets, resource sets and condition sets

/claim #47

@daveads
Copy link
Contributor Author

daveads commented Dec 17, 2024

permitcli.mp4

@daveads
Copy link
Contributor Author

daveads commented Dec 17, 2024

@gemanor

@gemanor
Copy link
Collaborator

gemanor commented Dec 29, 2024

Hey @daveads, seems like the CI failed due to build/lint/test issues, can you please test it?

@daveads
Copy link
Contributor Author

daveads commented Dec 29, 2024

Hey @daveads, seems like the CI failed due to build/lint/test issues, can you please test it?

okayy

@daveads
Copy link
Contributor Author

daveads commented Dec 30, 2024

@gemanor waiting for workflow approval.

Copy link
Collaborator

@gemanor gemanor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, in general this is a good start, but it has some core changes needed.

Please see the comments in the code itself, and also address the following:

  • The hooks/components shouldn't be in the command folder. Please use the other components/hooks folders for them
  • The HCL should have a better templating solution. It is better that they will be in .hcl files, and you'll use a template engine to render them. Some of the code that is rendering now
  • Some of the content now is rendered as object object instead of good stringify solution
  • Please run some tests with files that you exported and import it again using TF to ensure the exported file is indeed valid
  • There is no support with the depends_on for the resources, it could lead to race condition when working with terraform. Please add depends_on where relevant

@daveads
Copy link
Contributor Author

daveads commented Dec 30, 2024

Hey, in general this is a good start, but it has some core changes needed.

Please see the comments in the code itself, and also address the following:

* The hooks/components shouldn't be in the `command` folder. Please use the other `components`/`hooks` folders for them

* The HCL should have a better templating solution. It is better that they will be in `.hcl` files, and you'll use a template engine to render them. Some of the code that is rendering now

* Some of the content now is rendered as `object object` instead of good stringify solution

* Please run some tests with files that you exported and import it again using TF to ensure the exported file is indeed valid

* There is no support with the `depends_on` for the resources, it could lead to race condition when working with terraform. Please add `depends_on` where relevant

alright

Copy link
Collaborator

@gemanor gemanor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall structure looks good, and I like the approach you take in high-level.

In deeper look, there are many functions with too much intentation/lines. Left comments where it's not readable/testable and must refactor for better readability/testability.

Copy link
Collaborator

@gemanor gemanor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@gemanor gemanor merged commit 8f0cd05 into permitio:main Mar 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants