Print instruction after teardown #403

honnix · 2023-05-16T19:29:37Z

TL;DR

Print instruction after execution sandbox/demo teardown command.

This PR also includes a doc build fix similarly as flyteorg/flytekit#1619.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

As described in flyteorg/flyte#3689, it is helpful to guide users cleaning up.

Tracking Issue

Closes flyteorg/flyte#3689

Follow-up issue

NA

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2023-05-16T19:30:45Z

pkg/util/util.go

@@ -72,8 +72,14 @@ func SetupFlyteDir() error {
 	return nil
 }

-// PrintDemoMessage will print sandbox success message
+// PrintDemoMessage will print demo start success message
+// Deprecated: use PrintDemoStartMessage


If we are confident this is not used by any potential users, it is fine to delete.

I doubt that there's someone out there importing this. It's fine to delete.

honnix · 2023-05-16T19:30:53Z

pkg/util/util.go

@@ -97,8 +102,14 @@ func PrintDemoMessage(flyteConsolePort int, kubeconfigLocation string, dryRun bo
 	fmt.Printf("%s The Minio API is hosted on localhost:30002. Use http://localhost:30080/minio/login for Minio console\n", emoji.OpenFileFolder)
 }

-// PrintSandboxMessage will print sandbox success message
+// PrintSandboxMessage will print sandbox start success message
+// Deprecated: use PrintSandboxStartMessage


honnix · 2023-05-16T19:31:47Z

pkg/util/util.go

@@ -120,6 +130,12 @@ func PrintSandboxMessage(flyteConsolePort int, kubeconfigLocation string, dryRun
 	}
 }

+// PrintSandboxTeardownMessage will print sandbox teardown success message
+func PrintSandboxTeardownMessage(flyteConsolePort int, kubeconfigLocation string) {


It seems we don't need an equivalent for demo command because demo teardown invokes sandbox.Teardown()

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2023-05-16T19:33:56Z

pkg/util/util_test.go

+	})
+}
+
+func TestPrintSandboxTeardownMessage(t *testing.T) {


This test doesn't seem to be very useful, so just for the completeness.

in all honesty, the previous test is not terribly useful either. It's fine to leave both there.

honnix · 2023-05-16T19:36:58Z

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [6 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/tmp/pip-install-prtbml3a/sphinxcontrib-yt_82d7821052bb4688b3a0669cac992342/setup.py", line 4, in <module>
          from sphinxcontrib import yt as pkg
      ImportError: cannot import name 'yt' from 'sphinxcontrib' (unknown location)
      [end of output]

Doesn't seem to relate to this PR.

Signed-off-by: Hongxin Liang <[email protected]>

codecov · 2023-05-16T19:48:06Z

Codecov Report

Merging #403 (a5da5e5) into master (bd6b856) will increase coverage by 0.80%.
The diff coverage is 70.00%.

❗ Current head a5da5e5 differs from pull request most recent head eb015a1. Consider uploading reports for the commit eb015a1 to get more accurate results

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
+ Coverage   66.57%   67.37%   +0.80%     
==========================================
  Files         145      145              
  Lines        6271     5239    -1032     
==========================================
- Hits         4175     3530     -645     
+ Misses       1816     1429     -387     
  Partials      280      280

Flag	Coverage Δ
unittests	`?`

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

Impacted Files	Coverage Δ
pkg/sandbox/start.go	`56.15% <66.66%> (+2.42%)`	⬆️
pkg/util/util.go	`51.28% <66.66%> (+3.97%)`	⬆️
pkg/sandbox/teardown.go	`68.18% <100.00%> (+6.64%)`	⬆️

... and 131 files with indirect coverage changes

honnix · 2023-05-16T20:16:00Z

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [6 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/tmp/pip-install-prtbml3a/sphinxcontrib-yt_82d7821052bb4688b3a0669cac992342/setup.py", line 4, in <module>
          from sphinxcontrib import yt as pkg
      ImportError: cannot import name 'yt' from 'sphinxcontrib' (unknown location)
      [end of output]

Doesn't seem to relate to this PR.

This seems to be either a pip or setuptools issue.

A successful build has python -m pip install --upgrade --no-cache-dir pip setuptools<58.3.0, while a failed one has python -m pip install --upgrade --no-cache-dir pip setuptools.

I tried pip 23.0.1 and setuptools 58.2.0 locally and that worked. https://github.com/divi255/sphinxcontrib.youtube/blob/master/setup.py#L4 has the weird usage of importing itself from setup.py and maybe pip/setuptools no longer supports that.

Since this would require change to readthedocs, it is not possible to do anything in this PR.

eapolinario · 2023-05-17T21:38:48Z

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [6 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/tmp/pip-install-prtbml3a/sphinxcontrib-yt_82d7821052bb4688b3a0669cac992342/setup.py", line 4, in <module>
          from sphinxcontrib import yt as pkg
      ImportError: cannot import name 'yt' from 'sphinxcontrib' (unknown location)
      [end of output]
Doesn't seem to relate to this PR.
This seems to be either a pip or setuptools issue.

A successful build has python -m pip install --upgrade --no-cache-dir pip setuptools<58.3.0, while a failed one has python -m pip install --upgrade --no-cache-dir pip setuptools.

I tried pip 23.0.1 and setuptools 58.2.0 locally and that worked. https://github.com/divi255/sphinxcontrib.youtube/blob/master/setup.py#L4 has the weird usage of importing itself from setup.py and maybe pip/setuptools no longer supports that.

Since this would require change to readthedocs, it is not possible to do anything in this PR.

sphinxcontrib-youtube is supposed to be a dropin replacement (we did this in other repos, e.g.: flyteorg/flytekit#1619 )

honnix · 2023-05-17T22:00:14Z

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [6 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/tmp/pip-install-prtbml3a/sphinxcontrib-yt_82d7821052bb4688b3a0669cac992342/setup.py", line 4, in <module>
          from sphinxcontrib import yt as pkg
      ImportError: cannot import name 'yt' from 'sphinxcontrib' (unknown location)
      [end of output]
Doesn't seem to relate to this PR.
This seems to be either a pip or setuptools issue.
A successful build has python -m pip install --upgrade --no-cache-dir pip setuptools<58.3.0, while a failed one has python -m pip install --upgrade --no-cache-dir pip setuptools.
I tried pip 23.0.1 and setuptools 58.2.0 locally and that worked. https://github.com/divi255/sphinxcontrib.youtube/blob/master/setup.py#L4 has the weird usage of importing itself from setup.py and maybe pip/setuptools no longer supports that.
Since this would require change to readthedocs, it is not possible to do anything in this PR.
sphinxcontrib-youtube is supposed to be a dropin replacement (we did this in other repos, e.g.: flyteorg/flytekit#1619 )

🤦 , so many different sphinxcontrib youtube libs:

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2023-05-17T22:10:01Z

doc-requirements.txt

@@ -90,7 +90,7 @@ sphinxcontrib-qthelp==1.0.3
    # via sphinx
 sphinxcontrib-serializinghtml==1.1.5
    # via sphinx
-sphinxcontrib-yt==0.2.2
+sphinxcontrib-youtube==1.2.0


I did this manually. I was not able to do make doc-requirements.txt because pip-compile complained:

Using legacy resolver. Consider using backtracking resolver with `--resolver=backtracking`. Could not find a version that matches sphinx<5,<8.0.0,>=0.6,>=1.3,>=1.5.0,>=1.8,>=2,>=7.0.0,~=4.0 (from -r doc-requirements.in (line 2)) Tried: 0.1.61611, 0.1.61798, 0.1.61843, 0.1.61945, 0.1.61950, 0.2, 0.3, 0.4, 0.4.1, 0.4.2, 0.4.3, 0.5, 0.5.1, 0.5.2, 0.6, 0.6.1, 0.6.2, 0.6.3, 0.6.4, 0.6.5, 0.6.6, 0.6.7, 1.0, 1.0.1, 1.0.2, 1.0.3, 1.0.4, 1.0.5, 1.0.6, 1.0.7, 1.0.8, 1.1, 1.1.1, 1.1.2, 1.1.3, 1.2, 1.2, 1.2, 1.2.1, 1.2.1, 1.2.1, 1.2.2, 1.2.2, 1.2.2, 1.2.3, 1.2.3, 1.3, 1.3, 1.3.1, 1.3.1, 1.3.2, 1.3.2, 1.3.3, 1.3.3, 1.3.4, 1.3.4, 1.3.5, 1.3.5, 1.3.6, 1.3.6, 1.4, 1.4, 1.4.1, 1.4.1, 1.4.2, 1.4.2, 1.4.3, 1.4.3, 1.4.4, 1.4.4, 1.4.5, 1.4.5, 1.4.6, 1.4.6, 1.4.7, 1.4.7, 1.4.8, 1.4.8, 1.4.9, 1.4.9, 1.5, 1.5, 1.5.1, 1.5.1, 1.5.2, 1.5.2, 1.5.3, 1.5.3, 1.5.4, 1.5.4, 1.5.5, 1.5.5, 1.5.6, 1.5.6, 1.6.1, 1.6.1, 1.6.2, 1.6.2, 1.6.3, 1.6.3, 1.6.4, 1.6.4, 1.6.5, 1.6.5, 1.6.6, 1.6.6, 1.6.7, 1.6.7, 1.7.0, 1.7.0, 1.7.1, 1.7.1, 1.7.2, 1.7.2, 1.7.3, 1.7.3, 1.7.4, 1.7.4, 1.7.5, 1.7.5, 1.7.6, 1.7.6, 1.7.7, 1.7.7, 1.7.8, 1.7.8, 1.7.9, 1.7.9, 1.8.0, 1.8.0, 1.8.1, 1.8.1, 1.8.2, 1.8.2, 1.8.3, 1.8.3, 1.8.4, 1.8.4, 1.8.5, 1.8.5, 1.8.6, 1.8.6, 2.0.0, 2.0.0, 2.0.1, 2.0.1, 2.1.0, 2.1.0, 2.1.1, 2.1.1, 2.1.2, 2.1.2, 2.2.0, 2.2.0, 2.2.1, 2.2.1, 2.2.2, 2.2.2, 2.3.0, 2.3.0, 2.3.1, 2.3.1, 2.4.0, 2.4.0, 2.4.1, 2.4.1, 2.4.2, 2.4.2, 2.4.3, 2.4.3, 2.4.4, 2.4.4, 2.4.5, 2.4.5, 3.0.0, 3.0.0, 3.0.1, 3.0.1, 3.0.2, 3.0.2, 3.0.3, 3.0.3, 3.0.4, 3.0.4, 3.1.0, 3.1.0, 3.1.1, 3.1.1, 3.1.2, 3.1.2, 3.2.0, 3.2.0, 3.2.1, 3.2.1, 3.3.0, 3.3.0, 3.3.1, 3.3.1, 3.4.0, 3.4.0, 3.4.1, 3.4.1, 3.4.2, 3.4.2, 3.4.3, 3.4.3, 3.5.0, 3.5.0, 3.5.1, 3.5.1, 3.5.2, 3.5.2, 3.5.3, 3.5.3, 3.5.4, 3.5.4, 4.0.0, 4.0.0, 4.0.1, 4.0.1, 4.0.2, 4.0.2, 4.0.3, 4.0.3, 4.1.0, 4.1.0, 4.1.1, 4.1.1, 4.1.2, 4.1.2, 4.2.0, 4.2.0, 4.3.0, 4.3.0, 4.3.1, 4.3.1, 4.3.2, 4.3.2, 4.4.0, 4.4.0, 4.5.0, 4.5.0, 5.0.0, 5.0.0, 5.0.1, 5.0.1, 5.0.2, 5.0.2, 5.1.0, 5.1.0, 5.1.1, 5.1.1, 5.2.0, 5.2.0, 5.2.0.post0, 5.2.0.post0, 5.2.1, 5.2.1, 5.2.2, 5.2.2, 5.2.3, 5.2.3, 5.3.0, 5.3.0, 6.0.0, 6.0.0, 6.0.1, 6.0.1, 6.1.0, 6.1.0, 6.1.1, 6.1.1, 6.1.2, 6.1.2, 6.1.3, 6.1.3, 6.2.0, 6.2.0, 6.2.1, 6.2.1, 7.0.0, 7.0.0, 7.0.1, 7.0.1 Skipped pre-versions: 0.5.2b1, 0.6b1, 1.0b1, 1.0b2, 1.2b1, 1.2b2, 1.2b3, 1.3b1, 1.3b1, 1.3b2, 1.3b2, 1.3b3, 1.3b3, 1.4a1, 1.4a1, 1.4b1, 1.4b1, 1.5a1, 1.5a1, 1.5a2, 1.5a2, 1.5b1, 1.5b1, 1.6b1, 1.6b1, 1.6b2, 1.6b2, 1.6b3, 1.6b3, 1.7.0b1, 1.7.0b1, 1.7.0b2, 1.7.0b2, 1.8.0b1, 1.8.0b1, 2.0.0b1, 2.0.0b1, 2.0.0b2, 2.0.0b2, 3.0.0b1, 3.0.0b1, 4.0.0b1, 4.0.0b1, 4.0.0b2, 4.0.0b2, 5.0.0b1, 5.0.0b1, 6.0.0b1, 6.0.0b1, 6.0.0b2, 6.0.0b2, 7.0.0rc1, 7.0.0rc1 There are incompatible versions in the resolved dependencies: sphinx (from -r doc-requirements.in (line 2)) Sphinx<8.0.0,>=7.0.0 (from sphinx-prompt==1.7.0->-r doc-requirements.in (line 3)) Sphinx>=0.6 (from sphinxcontrib-youtube==1.2.0->-r doc-requirements.in (line 8)) sphinx~=4.0 (from furo==2022.4.7.dev1->-r doc-requirements.in (line 1)) sphinx<5,>=2 (from sphinx-panels==0.6.0->-r doc-requirements.in (line 9)) Sphinx>=1.5.0 (from sphinx_fontawesome==0.0.6->-r doc-requirements.in (line 7)) sphinx>=1.3 (from sphinx-code-include==1.1.1->-r doc-requirements.in (line 5)) sphinx>=2.0 (from sphinx-material==0.0.35->-r doc-requirements.in (line 4)) sphinx>=1.8 (from sphinx-copybutton==0.5.2->-r doc-requirements.in (line 6)) make: *** [doc-requirements.txt] Error 2

So sphinx-panels and sphinx-prompt don't agree with each other.

sphinx-panels is no longer maintained https://github.com/executablebooks/sphinx-panels#sphinx-panels :(

honnix · 2023-05-17T22:13:19Z

docs/source/conf.py

@@ -43,7 +43,7 @@
    "sphinx-prompt",
    "sphinx_copybutton",
    "sphinx_fontawesome",
-    "sphinxcontrib.yt",
+    "sphinxcontrib-youtube",


TIL this conf.py file.

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2023-05-17T22:16:38Z

docs/source/conf.py

@@ -43,7 +43,7 @@
    "sphinx-prompt",
    "sphinx_copybutton",
    "sphinx_fontawesome",
-    "sphinxcontrib.yt",
+    "sphinxcontrib.youtube",


sphinxcontrib-youtube vs. sphinxcontrib.youtube, lol.

Print instruction after teardown

db387cd

Signed-off-by: Hongxin Liang <[email protected]>

honnix requested review from kumare3, EngHabu and wild-endeavor as code owners May 16, 2023 19:29

honnix commented May 16, 2023

View reviewed changes

Add unit test

271647f

Signed-off-by: Hongxin Liang <[email protected]>

honnix commented May 16, 2023

View reviewed changes

Fix import

63d58c0

Signed-off-by: Hongxin Liang <[email protected]>

honnix force-pushed the teardown-print branch from 12be30a to 63d58c0 Compare May 16, 2023 19:44

honnix added 2 commits May 18, 2023 00:00

Delete unused functions

660d1cf

Signed-off-by: Hongxin Liang <[email protected]>

Try fixing doc build

1b29e37

Signed-off-by: Hongxin Liang <[email protected]>

honnix commented May 17, 2023

View reviewed changes

Name

eb015a1

Signed-off-by: Hongxin Liang <[email protected]>

honnix commented May 17, 2023

View reviewed changes

eapolinario approved these changes May 17, 2023

View reviewed changes

eapolinario merged commit 5cb23b7 into flyteorg:master May 17, 2023

honnix deleted the teardown-print branch May 24, 2023 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print instruction after teardown #403

Print instruction after teardown #403

honnix commented May 16, 2023 •

edited

Loading

honnix May 16, 2023

eapolinario May 17, 2023

honnix May 16, 2023

honnix May 16, 2023

honnix May 16, 2023

eapolinario May 17, 2023

honnix commented May 16, 2023

codecov bot commented May 16, 2023 •

edited

Loading

honnix commented May 16, 2023 •

edited

Loading

eapolinario commented May 17, 2023

honnix commented May 17, 2023

honnix May 17, 2023

honnix May 17, 2023

honnix May 17, 2023

honnix May 17, 2023

Print instruction after teardown #403

Print instruction after teardown #403

Conversation

honnix commented May 16, 2023 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honnix commented May 16, 2023

codecov bot commented May 16, 2023 • edited Loading

Codecov Report

honnix commented May 16, 2023 • edited Loading

eapolinario commented May 17, 2023

honnix commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honnix commented May 16, 2023 •

edited

Loading

codecov bot commented May 16, 2023 •

edited

Loading

honnix commented May 16, 2023 •

edited

Loading