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

feat: project create #115

Merged
merged 29 commits into from
Oct 19, 2022
Merged

feat: project create #115

merged 29 commits into from
Oct 19, 2022

Conversation

benPearce1
Copy link
Contributor

@benPearce1 benPearce1 commented Oct 14, 2022

[sc-18973]

This PR is currently blocked on PR OctopusDeploy/go-octopusdeploy#152 and the work on branch https://github.com/OctopusDeploy/go-octopusdeploy/tree/feat/git-credentials being merged

basic project create interaction:
image

basic project create with creating a project group inline:
image

Config-as-code project, with project stored credentials:
image

No-prompts:
image

@benPearce1 benPearce1 requested a review from a team October 14, 2022 07:06
@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #18973: CLI vNext: project create.

@benPearce1 benPearce1 marked this pull request as draft October 14, 2022 07:07
FlagName = "name"
FlagDescription = "description"
FlagLifecycle = "lifecycle"
FlagConfigAsCode = "cac"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I am happy with name. Suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found when doing release create that it's referred to as "Version Controlled" in almost all places in the octopus codebase and web portal and other documentation. E.g. you'll see documentation snippets like "for a version controlled project...", so I used that term there.
So, within the CLI there's precedent to call this "versionControlled".

On the other hand, the "brand name" that keeps showing up on twitter/linkedin/customer forums is always Config As Code, so 🤷 ? tag @jbristowe

go.mod Outdated Show resolved Hide resolved
"github.com/spf13/cobra"
)

type Dependable interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I completely understand we're trying to fit within the patterns and idioms of golang... but Dependable is perhaps too vague. Does this mean it's never going to let us down? 🤣

GenerateAutomationCmd()
}

type Dependencies struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps Dependencies might be a bit of a vague name here? My immediate thought is "which dependencies does this struct represent? who depends on them and why?"

From a quick look, it appears as though this struct is meant to carry services (client, asker) and options (space, noprompt) that are commonly used across many commands? If so, some possible naming suggestions?

  • ServicesAndOptions
  • CommandContext
  • CliCommandContext
  • CommandCommonData
  • StandardCommandProperties
  • CommonCommandProperties
  • GeneralCommandProps

... something to think about anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

tag @slewis74

@@ -0,0 +1,57 @@
package cmd
Copy link
Collaborator

Choose a reason for hiding this comment

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

q for @domenicsim1
Given that this isn't a command itself, should it go under the cmd directory structure, or should it go under just pkg or pkg/somethingelse?

Copy link
Collaborator

@domenicsim1 domenicsim1 Oct 18, 2022

Choose a reason for hiding this comment

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

maybe, although it is suppose to be the basis of cmds. Not sure yet, happy to leave it for now.

pkg/cmd/project/create/create.go Outdated Show resolved Hide resolved
FlagName = "name"
FlagDescription = "description"
FlagLifecycle = "lifecycle"
FlagConfigAsCode = "cac"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found when doing release create that it's referred to as "Version Controlled" in almost all places in the octopus codebase and web portal and other documentation. E.g. you'll see documentation snippets like "for a version controlled project...", so I used that term there.
So, within the CLI there's precedent to call this "versionControlled".

On the other hand, the "brand name" that keeps showing up on twitter/linkedin/customer forums is always Config As Code, so 🤷 ? tag @jbristowe

}, &opts.GitPassword.Value, survey.WithValidator(survey.ComposeValidators(
survey.MaxLength(200),
survey.MinLength(1),
survey.Required,
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC both Required and MinLength(1) do the same job, and we probably don't need both of them.

I ran into this when selecting environments for release deployment, which is a multi-select with min-items(1); Required did the same job but produced a better error message so I used that instead

pkg/cmd/project/create/create.go Show resolved Hide resolved
pkg/cmd/project/create/create.go Outdated Show resolved Hide resolved
@benPearce1 benPearce1 marked this pull request as ready for review October 18, 2022 02:17
Copy link
Collaborator

@domenicsim1 domenicsim1 left a comment

Choose a reason for hiding this comment

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

🏆 👍

@@ -0,0 +1,57 @@
package cmd
Copy link
Collaborator

@domenicsim1 domenicsim1 Oct 18, 2022

Choose a reason for hiding this comment

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

maybe, although it is suppose to be the basis of cmds. Not sure yet, happy to leave it for now.

Comment on lines +97 to +104
testutil.NewConfirmPrompt("Would you like to use Config as Code?", "", true),
testutil.NewSelectPrompt("Select where to store the Git credentials", "", []string{"Library", "Project"}, "Project"),
testutil.NewInputPrompt("Git URL", "The URL of the Git repository to store configuration.", "https://github.com/blah.git"),
testutil.NewInputPrompt("Git repository base path", "The path in the repository where Config As Code settings are stored. Default value is '.octopus/'.", "./octopus/project"),
testutil.NewInputPrompt("Git branch", "The default branch to use. Default value is 'main'.", "main"),
testutil.NewInputPrompt("Initial Git commit message", "The commit message used in initializing. Default value is 'Initial commit of deployment process'.", "init message"),
testutil.NewInputPrompt("Git username", "The Git username.", "user1"),
testutil.NewPasswordPrompt("Git password", "The Git password.", "password"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

🧑‍🍳 💋


func getGitBranch(opts *CreateOptions) string {
if opts.GitBranch.Value == "" {
return "main"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lots of people have git branches where the default is 'master' so this might cause them problems?
I'm not sure how much effort we want to go to in solving this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

main is the default in the UI and also the default when you create a new repo in GitHub, which has been the case for a couple of years (https://github.blog/changelog/2020-10-01-the-default-branch-for-newly-created-repositories-is-now-main/).
At Octopus, we also went through a process of swapping the default branch to main where it was easy to do so. Server is still using master, but most other repos are main

Copy link
Collaborator

@borland borland Oct 19, 2022

Choose a reason for hiding this comment

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

For sure, but most companies are not nearly so forward-thinking as Octopus 😄 There's still a ton of 'master' out there. I can't say if this matters or not, but it'd be good to raise the question in a standup, if you haven't already, that's all. Cheers

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