Skip to content

Re-implement init UI using terminal UI components (based on PTerm.sh)#400

Merged
ezodude merged 17 commits intomasterfrom
es-init-ui-components-36
Mar 18, 2021
Merged

Re-implement init UI using terminal UI components (based on PTerm.sh)#400
ezodude merged 17 commits intomasterfrom
es-init-ui-components-36

Conversation

@ezodude
Copy link
Contributor

@ezodude ezodude commented Mar 9, 2021

Resolves https://github.com/appvia/kev-planning/issues/36

Please see ticket for proposed UI mockup.

To test this,

  • Run make.
  • Run kev init. (The tests are broken but the binary works)

Pending Items - COMPLETED

  • Fix broken tests.
  • Clean up of legacy project init.
  • No-op UI.
  • Tests for new Project init sequences.
  • fake UI for testing.
  • Add tests to ensure UI reporting works as expected.

@ezodude ezodude force-pushed the es-init-ui-components-36 branch from 87e5d67 to 8b06e99 Compare March 9, 2021 18:04
@ezodude ezodude marked this pull request as draft March 9, 2021 18:05
@ezodude ezodude requested review from mangas and marcinc March 9, 2021 18:08
@ezodude ezodude marked this pull request as ready for review March 11, 2021 22:29
@ezodude ezodude marked this pull request as draft March 11, 2021 22:29
@ezodude ezodude force-pushed the es-init-ui-components-36 branch 3 times, most recently from ffce808 to 16b8409 Compare March 15, 2021 15:17
@ezodude ezodude marked this pull request as ready for review March 15, 2021 15:49
@ezodude ezodude force-pushed the es-init-ui-components-36 branch from 9ac514b to 67beab0 Compare March 15, 2021 15:53
@ezodude ezodude force-pushed the es-init-ui-components-36 branch 4 times, most recently from b98a459 to c503b34 Compare March 16, 2021 14:38
@ezodude ezodude force-pushed the es-init-ui-components-36 branch from c503b34 to 0cc6f96 Compare March 16, 2021 14:53
Copy link
Member

@marcinc marcinc left a comment

Choose a reason for hiding this comment

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

A few comments but generally loving the changes in this PR 💯

return p.manifest
}

func WithComposeSources(c []string) Options {
Copy link
Member

Choose a reason for hiding this comment

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

A small potential improvement... We could use method chain on Project instead? then you could do something like Init().WithComposeSources(...).WithEvns(...).WithSkaffold(...).WithUI(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcinc ... To clarify:

Do you mean instead of passing opts to Init(). And, creating extra methods on Project to perform the wiring?

But how would you know which method (.WithComposeSources(...) or .WithEvns) to call?

Copy link
Member

Choose a reason for hiding this comment

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

yeh, that's what I meant. Current implementation is correct, just thought that method chain could be slightly nicer. To your question, I think you'd know which method to chain the same way you know it now.. an excertp:

	return kev.InitProjectWithOptions(wd,
		kev.WithComposeSources(files),
		kev.WithEnvs(envs),
		kev.WithSkaffold(skaffold))

# this would become

	return kev.InitProject(wd).WithComposeSources(files).WithEnvs(envs).WithSkaffold(skaffold)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh, I would rather leave it as is.

Otherwise, you would need to:
-> kev.InitProject(wd) to return a runner object.
-> Configure using With... methods.

... And you also then:
-> have to Run() the runner.
-> and do the other things that InitProjectWithOptions is doing for you.

Copy link
Member

Choose a reason for hiding this comment

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

as said, it's good as is. If chaining brings extra complexity then that's fine.

pkg/kev/init.go Outdated
}
updateStep.Success(time.Second * 1)
case false:
createStep := sg.Add(fmt.Sprintf("Creating Skaffold config with deployment environments at: %s", skPath))
Copy link
Member

Choose a reason for hiding this comment

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

with deployment environments at ... -> with deployment environment profiles at ...

ezodude added 2 commits March 17, 2021 21:29
Removed unnecessary delays.

Introduced a minimum display lag for step success, warning and error.

Updated Skaffold messaging.
@ezodude ezodude force-pushed the es-init-ui-components-36 branch from 847c810 to da034ac Compare March 18, 2021 11:49
pkg/kev/init.go Outdated
}

s.Success(time.Second * 1)
s.Success(0)
Copy link
Member

Choose a reason for hiding this comment

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

makes sense for now I guess. Later we could have it managed centrally / or locally when needed by chaining .Wait(time.Seconds * 1) or similar. Could be done in a follow up PR as discussed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is outdated.

@ezodude ezodude requested a review from marcinc March 18, 2021 14:32
Copy link
Member

@marcinc marcinc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ezodude ezodude merged commit e723e4f into master Mar 18, 2021
@ezodude ezodude deleted the es-init-ui-components-36 branch March 18, 2021 16:01
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.

2 participants