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

Implement deployments create #221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rustand2
Copy link

@rustand2 rustand2 commented Oct 2, 2023

Add a command to create new deployments from mender-cli. I'm sorry if this is rather rough, but I've never used go before. Currently the only deployment command I implemented was create, and there is limited handling of wrong arguments. Currently the device list needs to be passed as a raw json list by the user which is probably not an acceptable way to do it. I would appreciate any help to make this better from someone more proficient in go.

@rustand2 rustand2 force-pushed the master branch 5 times, most recently from cc2949c to 065665e Compare October 2, 2023 13:01
@mender-test-bot
Copy link

Merging these commits will result in the following changelog entries:

Changelogs

mender-cli (master)

New changes in mender-cli since master:

Features
  • add deployments create

@rustand2 rustand2 force-pushed the master branch 8 times, most recently from f41204e to be04eb4 Compare October 2, 2023 13:27
Added a new command to create deployments.

Changelog: title
Ticket: none
Signed-off-by: Lars Rustand <[email protected]>
Copy link
Member

@kacf kacf left a comment

Choose a reason for hiding this comment

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

Thanks @rustand2!

I didn't do a detailed review now, I focused on the high level design. I think it looks alright, but I have requested some changes to improve the command line interface.

Comment on lines +60 to +71
if len(args) > 1 {
deploymentName = args[0]
}

artifactID := ""
if len(args) > 2 {
artifactID = args[1]
}

deviceList := ""
if len(args) >= 3 {
deviceList = args[2]
Copy link
Member

Choose a reason for hiding this comment

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

I think having mandatory ordered arguments, with no flags, is not good style, and difficult to change or adapt later. There should be --deployment-name <NAME> and -n <NAME> type flags for all of these I think. Take a look at how the other commands are adding arguments.

)

var deploymentCreateCmd = &cobra.Command{
Use: "create [flags] DEPLOYMENT_NAME ARTIFACT_NAME DEVICE_LIST",
Copy link
Member

Choose a reason for hiding this comment

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

This should be documented better, especially device list. Like where it can be found, and whether the list is comma separated or not. I know this comes from the API, but people don't typically look there.

@TheYoctoJester
Copy link

Any news on this @rustand2? Is there something we can help with putting this forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants