-
Notifications
You must be signed in to change notification settings - Fork 14
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
refactor: restructure organization commands #332
Conversation
You'll need to tidy up the docs directory. There are references to the old commands still: https://github.com/uselagoon/lagoon-cli/blob/remove-org-subcommands/docs/commands/lagoon_add_organization_deploytarget.md (plus others) Unfortunately the docs command doesn't clean this up for you. The simple solution is to just delete anything in the |
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.
Will need updates based on latest merges to main, and also the documentation fixes.
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.
Added some comments and such around some things, if you're not sure about something though, ask
Sorry to hijack the PR a bit just then. I saw a few things that I just went and fixed up. I'll do a proper run through with this today and see if it is all good though |
…/lagoon-cli into remove-org-subcommands
cmd/deploytarget.go
Outdated
|
||
deleteDeployTargetCmd.Flags().UintP("id", "", 0, "ID of the DeployTarget") | ||
deleteDeployTargetCmd.Flags().StringP("name", "", "", "Name of DeployTarget") | ||
|
||
RemoveDeployTargetFromOrganizationCmd.Flags().StringP("name", "O", "", "Name of Organization") | ||
RemoveDeployTargetFromOrganizationCmd.Flags().UintP("deploy-target", "D", 0, "ID of DeployTarget") | ||
removeDeployTargetFromOrganizationCmd.Flags().StringP("name", "O", "", "Name of Organization") |
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.
Can we change this to organization
to match other flags in other commands. There may be other organization commands that use name, but if they can be updated to be organization
that'd be wicked. That way there is no confusion across the flagset for organizations.
It was initially confusing to me when i went to use --name
in one place (lagoon list organization-users --organization ${orgname}
) only to find it was --organization
and then vice versa when using a different command (lagoon delete organization-deploytarget --name ${orgname}
).
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.
I think the only one that should be --name
is maybe add organization
.
Alternatively, if the flag is changed to organization-name
then we cover all possibly cases on all commands
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.
Sounds good, I've changed the flag to organization-name
.
Checklist
Moves organization subcommands to the top level, prefixes a number of cmds with 'organization-' & allows projects & groups to be created in an organization via the
--organization-name
flag in the currentaddProject
/addGroup
commands. Rewrites a few of the legacy commands to utilize machinery.E.g.
lagoon add organization organization --name
lagoon add organization --organization-name
lagoon add organization deploytarget
lagoon add organization-deploytarget
lagoon add organization project
lagoon add project --organization-name etc