export command #54
export command #54
Conversation
permitcli.mp4 |
|
Hey @daveads, seems like the CI failed due to build/lint/test issues, can you please test it? |
okayy |
|
@gemanor waiting for workflow approval. |
There was a problem hiding this comment.
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
commandfolder. Please use the othercomponents/hooksfolders for them - The HCL should have a better templating solution. It is better that they will be in
.hclfiles, 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 objectinstead 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_onfor the resources, it could lead to race condition when working with terraform. Please adddepends_onwhere relevant
| const key = apiKey || authToken; | ||
| const { state, setState, exportConfig } = useExport(key); | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
Please use the convention of importing the function and use them without the React.
import React, { useEffect } from 'react';
...
useEffect(() => {
| return; | ||
| } | ||
| } else { | ||
| console.log(hcl); |
There was a problem hiding this comment.
This should be printed as part of render function, using the console.log here can be confusing and lead to unmaintainable code
There was a problem hiding this comment.
render function, means to render it as part of the command/component return section
| } | ||
|
|
||
| export const ExportStatus: React.FC<ExportStatusProps> = ({ state, file }) => { | ||
| if (state.error) { |
There was a problem hiding this comment.
Please replace the if (state.error) { convention with {state.error && convention, to make it more readable
| () => | ||
| new Permit({ | ||
| token, | ||
| pdp: 'http://localhost:7766', |
There was a problem hiding this comment.
This variable should be configurable from the function arguments to allow configurable pdp URL for further usages
| try { | ||
| const warningCollector = createWarningCollector(); | ||
|
|
||
| let hcl = `# Generated by Permit CLI |
There was a problem hiding this comment.
Please move all the .hcl code to static .hcl files in a dedicated folder and import them to the code where needed.
It will:
- Made the code more readable and keep the indentation
- Ensure reusable of code snippets
There was a problem hiding this comment.
It is up to you to decide on the import way of this text and use the templates in it
| import { Permit } from 'permitio'; | ||
| import React from 'react'; | ||
|
|
||
| export const PermitSDK = (token: string) => { |
There was a problem hiding this comment.
Please use the hook naming convention usePermitSDK
| key = "${resource.key}" | ||
| name = "${resource.name}"${ | ||
| resource.description ? `\n description = "${resource.description}"` : '' | ||
| }${resource.urn ? `\n urn = "${resource.urn}"` : ''} |
There was a problem hiding this comment.
This is redundant probably
alright |
feat: add terraform environment export command
permit env export terraformcommand/claim #47