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

[Monorepo] Bring flytectl #5301

Merged
merged 358 commits into from
Apr 30, 2024
Merged

[Monorepo] Bring flytectl #5301

merged 358 commits into from
Apr 30, 2024

Conversation

eapolinario
Copy link
Contributor

Tracking issue

NA

Why are the changes needed?

Having flytectl in the monorepo helps in the development and rollout of changes that require cross-component changes (including flyteidl).

What changes were proposed in this pull request?

Similar to #4017, this PR only brings in flytectl at flyteorg/flytectl@67cae23. Subsequent PRs will hook up the CI checks and necessary changes to release flytectl on Flyte releases.

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

flyte-bot and others added 30 commits July 13, 2021 00:24
Signed-off-by: Flyte-Bot <[email protected]>

Co-authored-by: evalsocket <[email protected]>
* fix sandbox start if sandbox exist

Signed-off-by: Yuvraj <[email protected]>
* Added node execution data to show inputs and outputs

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Added closure class

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Removed the old wrapper classes

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Uncommented the task exec closure

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Fixed unit tests for the closure classes

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* go mod tidy

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Adding .gitattribute file to contain rule for .rst files

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Adding gen path to .gitattr

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Added comments to new structs

Signed-off-by: Prafulla Mahindrakar <[email protected]>
* fix gcp parameters... s3 -> google

Signed-off-by: Bruce Arctor <[email protected]>

* change s3 to google for gcs docs

Signed-off-by: Bruce Arctor <[email protected]>
Signed-off-by: Flyte-Bot <[email protected]>

Co-authored-by: kumare3 <[email protected]>
* Added setup flytedir before config init

Signed-off-by: Yuvraj <[email protected]>
* code-of-conduct

Signed-off-by: Samhita Alla <[email protected]>

* boilerplate

Signed-off-by: Samhita Alla <[email protected]>
* Added k8s check in sandbox
* Added upgrade command

Signed-off-by: Yuvraj <[email protected]>
Signed-off-by: Flyte-Bot <[email protected]>

Co-authored-by: pmahindrakar-oss <[email protected]>
* Added version compare check and bug fix in sandbox start

Signed-off-by: Yuvraj <[email protected]>
Signed-off-by: Arnaud Alies <[email protected]>
Signed-off-by: Flyte-Bot <[email protected]>

Co-authored-by: evalsocket <[email protected]>
Update current video to a shorter version specifically edited to be embedded in the docs.
…#168)

`flytectl get task/workflow/launchplan -o table` panics if resource has no expected output.
We encountered `FormatVariableDescriptions at pkg/printer/printer.go:173` to panic when using
the get commands on a resource without expected output.
This happened with both explicit table output as well as default.

Replicated on lastest master, workflow demo-1.no-output has no expected output:

```
$ go run ./main.go get launchplans -p demo-1 -d production --latest demo-1.no-output

panic: assignment to entry in nil map

goroutine 1 [running]:
github.com/flyteorg/flytectl/pkg/printer.FormatVariableDescriptions(0x0)
	/workspace/flyteorg/flytectl/pkg/printer/printer.go:202 +0x307
github.com/flyteorg/flytectl/cmd/get.LaunchplanToTableProtoMessages({0xc000a30938, 0x1, 0x7ffeefbff977})
	/workspace/flyteorg/flytectl/cmd/get/launch_plan.go:136 +0xee
github.com/flyteorg/flytectl/cmd/get.getLaunchPlanFunc({0x2dfbf98, 0xc0000580b8}, {0xc0002fb8c0, 0x1, 0x11519b4}, {{0x2e55850, 0xc000a30120}, {0x2e472c0, 0xc0009a3110}, {0x2ddff60, ...}, ...})
	/workspace/flyteorg/flytectl/cmd/get/launch_plan.go:158 +0x307
github.com/flyteorg/flytectl/cmd/core.generateCommandFunc.func1(0xc000481680, {0xc0002fb8c0, 0x1, 0x6})
	/workspace/flyteorg/flytectl/cmd/core/cmd.go:69 +0x47d
github.com/spf13/cobra.(*Command).execute(0xc000481680, {0xc0002fb860, 0x6, 0x6})
	/workspace/go/pkg/mod/github.com/spf13/[email protected]/command.go:852 +0x60e
github.com/spf13/cobra.(*Command).ExecuteC(0xc000480000)
	/workspace/go/pkg/mod/github.com/spf13/[email protected]/command.go:960 +0x3ad
github.com/spf13/cobra.(*Command).Execute(...)
	/workspace/go/pkg/mod/github.com/spf13/[email protected]/command.go:897
github.com/flyteorg/flytectl/cmd.ExecuteCmd()
	/workspace/flyteorg/flytectl/cmd/root.go:135 +0x1e
main.main()
	/workspace/flyteorg/flytectl/main.go:12 +0x1d
exit status 2
```

Simple fix:
nil checks exists for container but not on the map itself prior to calling, and nil input causes the panic.
Added nil check on map parameter prior to calling in all places found.

Signed-off-by: Viktor Gerdin <[email protected]>
* Added launchplan update command and moved namedentity

Signed-off-by: Prafulla Mahindrakar <[email protected]>
asoundarya96 and others added 19 commits December 26, 2023 11:49
* Rename --archive to --deactivate in update launchplan

Signed-off-by: asoundarya96 <[email protected]>

* Rename --archive to --deactivate in update launchplan

Signed-off-by: asoundarya96 <[email protected]>

* Keep --archive as deprecated flag

Signed-off-by: asoundarya96 <[email protected]>

* make generate

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: asoundarya96 <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
* project-archive-with-yaml

Signed-off-by: Peeter Piegaze <[email protected]>

* fix syntax

Signed-off-by: Peeter Piegaze <[email protected]>

* fix list

Signed-off-by: Peeter Piegaze <[email protected]>

* update generated

Signed-off-by: Peeter Piegaze <[email protected]>

---------

Signed-off-by: Peeter Piegaze <[email protected]>
* feat: add pagination for get execution (draft)

Signed-off-by: zychen5186 <[email protected]>

fix: catches existing commands in os.Args

Signed-off-by: zychen5186 <[email protected]>

fix: restore neccessary codes

Signed-off-by: zychen5186 <[email protected]>

* feat: use -i to trigger bubbletea, add pagination for get execution

Signed-off-by: zychen5186 <[email protected]>

* change dot to arabic paging format

Signed-off-by: zychen5186 <[email protected]>

change dot to arabic paging format

Signed-off-by: zychen5186 <[email protected]>

change var names

Signed-off-by: zychen5186 <[email protected]>

fix: lint

Signed-off-by: zychen5186 <[email protected]>

* reuse JSONToTable

Signed-off-by: zychen5186 <[email protected]>

* reuse JSONToTable

Signed-off-by: zychen5186 <[email protected]>

change := to var

Signed-off-by: zychen5186 <[email protected]>

* keep original format when not using bubbletea

Signed-off-by: zychen5186 <[email protected]>

* improve readability and no functionality is changed

Signed-off-by: zychen5186 <[email protected]>

---------

Signed-off-by: zychen5186 <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
…pare-monorepo--flytectl

Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario eapolinario changed the title Prepare monorepo flytectl [Monorepo] Bring flytectl Apr 30, 2024
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.19%. Comparing base (40dccab) to head (8cad4ed).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5301      +/-   ##
==========================================
+ Coverage   59.68%   60.19%   +0.50%     
==========================================
  Files         568      646      +78     
  Lines       41729    45654    +3925     
==========================================
+ Hits        24906    27480    +2574     
- Misses      14410    15582    +1172     
- Partials     2413     2592     +179     
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.78% <ø> (-0.05%) ⬇️
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flyteidl 79.30% <ø> (ø)
unittests-flyteplugins 61.94% <ø> (ø)
unittests-flytepropeller 57.32% <ø> (ø)
unittests-flytestdlib 65.73% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Eduardo Apolinario <[email protected]>
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

amazing, thank you!

)

replace (
github.com/flyteorg/flyte/flyteidl => github.com/flyteorg/flyte/flyteidl v1.9.12
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be relative now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, module renaming is next.

Copy link
Contributor

Choose a reason for hiding this comment

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

still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, will drop in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this can be handled in a follow-up, but do we still need all the package-level boilerplate with this move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, these all will be removed in separate PRs.

@eapolinario eapolinario merged commit 79a1992 into master Apr 30, 2024
48 checks passed
@eapolinario eapolinario deleted the prepare-monorepo--flytectl branch April 30, 2024 22:37
@ddl-ebrown
Copy link
Contributor

Oh cool... will hold off on any work for flyteorg/flytectl#472 and reevaluate for vulns after the move is complete

@eapolinario
Copy link
Contributor Author

thank you, @ddl-ebrown!

@troychiu
Copy link
Member

cc @zychen5186

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.