-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: env management commands #37
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # source/commands/login.tsx # source/lib/api.ts
@35C4n0r the lint has some errors.. |
Hey, some functional comments:
|
Please merge from main |
# Conflicts: # .gitignore # package-lock.json # package.json
Hey, @35C4n0r, the |
@gemanor, pushed a change that fixes the issue, It now handles the case when |
@35C4n0r, the scope is redundnat (just use the default), and the description/strategy should be also in the wizard, not only as variables |
@gemanor done! |
The tests are failing now. Also, the success message is missing from the copy command. |
There are also many linting warning. Please ensure you're fixing the useEffect dependencies problems. Just use the standard practices like useCallback/useMemo and pure functions to avoid such. |
@gemanor fixed the error |
As for the warnings, if I don't want something to be the dependency of a useEffect, why should I add it in the dependency array? |
@gemanor, fixed the warnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Appreciate the effort!
Please address the comments and attach a sanity test video for the following flows after addressing them)
- Environment copy
- To new one with a wizard
- To a new one with variables provided (all provided, and partially provided - see CR)
- To an existing environment that exists
- To an existing environment that does not exist (fail)
- Invite member
- With organization-level key
- With project-level key
- With wrong key
- With environment-level key
- With provided project/environment variables (see CR)
- With an email that is already a member
- Environment select
- With all optional values as variables
- Login flow on Windows (you said something about the port?)
} from '../../components/EnvironmentSelection.js'; | ||
import SelectInput from 'ink-select-input'; | ||
|
||
export const options = zod.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure of the inputs needs to make true sense for consistent flow.
Here are the following variables that should be supported:
- key(string) -
API Key to be used for the environment copying (should be at least a project level key)
- from(string) -
Optional: set the environment ID to copy from. In case not set, the CLI lets you select one.
- name(string) -
Optional: The environment name to copy to. In case not set, the CLI will ask you for one.
- description(string) -
Optional: The new environment description. In case not set, the CLI will ask you for it.
- to(string) -
Optional: copy the environment to an existing environment. In case this variable is set, the 'name' and 'description' variables will be ignored
' - conflictStrategy(enum | "fail" "overwrite") -
Optional: Set the environment conflict strategy. In case not set, will use "fail"
The logic should be:
- If all variables are provided (name/description or existing), just perform the copy
- If any of the variables are provided, skip the step of configuring it
- If the existing provided together with name/description, show an error
- If the existing id is provided and it's not exist, show an error
- Two steps now are redundant:
- Choose existing environment (should be provided as variable, if not its mean the wizard should copy the new one)
- Choose conflict strategy (default should be fail, if someone wants to change it, they need to pass it as a variable)
// checks if the api_key scope >= project_level & | ||
// sets the apiKey and sets the projectFrom | ||
|
||
const validateApiKeyScope = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hook should be external and be shared with other project-level commands such as select
}, [error, state]); | ||
|
||
useEffect(() => { | ||
if (apiKey && tokenType(apiKey) === TokenType.APIToken) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in the previous component. This function should be an external hook as it's share logic between many components.
environment_id: string | null; | ||
} | ||
|
||
export const useApiKeyApi = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be use for all the API Key level errors etc.
@@ -0,0 +1,40 @@ | |||
import { apiCall } from '../lib/api.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not being used anywhere
@35C4n0r any updates on that? |
Closes #18
/claim #18