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

Add apps detect command #1227

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pulkit-khullar
Copy link
Contributor

@pulkit-khullar pulkit-khullar commented Sep 8, 2022

The PR adds a sub-command Detect under apps. The command allows to import of a functions project and returns the AppSpec for that project.

Later then AppSpec can be used to create an Apps project.

@pulkit-khullar pulkit-khullar marked this pull request as draft September 8, 2022 06:10
@pulkit-khullar pulkit-khullar changed the title (WIP) SERVERLESS-1688 - Add apps detect command (WIP) Add apps detect command Sep 8, 2022
@pulkit-khullar pulkit-khullar marked this pull request as ready for review September 8, 2022 10:13
@pulkit-khullar pulkit-khullar changed the title (WIP) Add apps detect command Add apps detect command Sep 8, 2022
Copy link
Contributor

@nicktate nicktate left a comment

Choose a reason for hiding this comment

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

This is looking good so far, left a first round of comments inline. We don't have to do it as part of this PR but we should consider integrating with the new commands/charm library for interactive elements

args.go Outdated Show resolved Hide resolved

appSpec.Name = name
var funcSpecArray []*godo.AppFunctionsSpec
for _, component := range resp.Components {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a warning message stating other component types are not yet supported if we encounter them in the spec.

// output is global flag
detect := CmdBuilder(cmd,
RunAppsDetect, "detect", "Detect functions", "Detect functions project and convert it into apps project by adding the AppSpec.", Writer, aliasOpt("dt"))
AddStringFlag(detect, doctl.ArgProjectSource, "", "", `Project source.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check if we are currently in a git project directory and attempt to default to that source repo.

commands/apps.go Outdated Show resolved Hide resolved
@@ -119,6 +121,86 @@ func (s *appsService) Delete(appID string) error {
return err
}

func (s *appsService) Detect(source string, sha string, name string, branch string, autoDeploy bool) (*godo.AppSpec, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should consider changing this signature to only inline required parameters and put the rest in an options struct. e.g.:

type DetectOpts struct {
  Name string // random name generated by default
  Sha string // latest sha in branch by default
  AutoDeploy bool // False by default
}

func (s *appsService) Detect(source, branch string, opts DetectOpts) (*godo.AppSpec, error) {

if err != nil {
return err
}
if len(c.Args) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need argument parsing here if we are including auto-deploy as a flag as well.

}

switch Output {
case "json":
Copy link
Contributor

Choose a reason for hiding this comment

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

We should support YAML here as well

e.SetIndent("", " ")
return e.Encode(spec)
case "text":
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

(assuming this is just not implemented yet) just a reminder to either error out if not supported or implemented output support

args.go Outdated Show resolved Hide resolved
args.go Show resolved Hide resolved
@ChiefMateStarbuck ChiefMateStarbuck dismissed a stale review via 5053433 March 28, 2023 21:56
ChiefMateStarbuck and others added 3 commits March 28, 2023 16:56
Co-authored-by: Nick Tate <[email protected]>
Co-authored-by: Nick Tate <[email protected]>
Co-authored-by: Nick Tate <[email protected]>
@ChiefMateStarbuck
Copy link
Contributor

This PR has been de-prioritized and will receive the necessary attention when capacity is allotted.

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.

3 participants