Remove Houston version check on upgrade #353

adam2k · 2020-08-07T20:36:33Z

Description

astro upgrade should not require Houston context.

🎟 Issue(s)

Resolves astronomer/issues#1606

🧪 Functional Testing

You can test locally by build a local version that is the same as the latest:

Without authentication to a server:

$ ./astro upgrade
Astro CLI Version: v0.17.0 (2020.07.24)
Astro CLI Latest: v0.17.0  (2020.07.24)
Astro Server Version: Please authenticate to a cluster to see server version
You are running the latest version.

With server authentication:

./astro upgrade
Astro CLI Version: v0.17.0 (2020.07.24)
Astro CLI Latest: v0.17.0  (2020.07.24)
Astro Server Version: 0.18.0
You are running the latest version.

You can test locally by building a local version that using an outdated CurrVersion:

Without server authentication:

$ rm -rf ~/.astro
$ go build -o astro -ldflags "-X github.com/astronomer/astro-cli/version.CurrVersion=0.15.0  -X github.com/astronomer/astro-cli/version.CurrCommit=5ae31076d9f2dfc874c533e6c729e1fa7681b7f4" main.go
$./astro upgrade
Astro CLI Version: v0.15.0 (2020.06.01)
Astro CLI Latest: v0.17.0  (2020.07.24)
Astro Server Version: Please authenticate to a cluster to see server version
A newer version of the Astronomer CLI is available.
To upgrade to latest, run:
	$ curl -sL https://install.astronomer.io | sudo bash
OR for homebrew users:
	$ brew install astronomer/tap/astro

With server authentication:

$ ./astro upgrade
Astro CLI Version: v0.15.0 (2020.06.01)
Astro CLI Latest: v0.17.0  (2020.07.24)
Astro Server Version: 0.18.0
A newer version of the Astronomer CLI is available.
To upgrade to latest, run:
	$ curl -sL https://install.astronomer.io | sudo bash
OR for homebrew users:
	$ brew install astronomer/tap/astro

📋 Checklist

Rebased from the main (or release if patching) branch (before testing)
Ran make test before taking out of draft
Added/updated applicable tests
Tested against Houston-API
Communicated to/tagged owners of respective clients potentially impacted by these changes.
Updated any related documentation

codecov · 2020-08-07T20:36:39Z

Codecov Report

Merging #353 into main will increase coverage by 8.65%.
The diff coverage is 63.79%.

@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   43.40%   52.06%   +8.65%     
==========================================
  Files          24       31       +7     
  Lines        1744     2328     +584     
==========================================
+ Hits          757     1212     +455     
- Misses        957     1038      +81     
- Partials       30       78      +48

Impacted Files	Coverage Δ
airflow/airflow.go	`62.16% <0.00%> (+62.16%)`	⬆️
cmd/auth.go	`44.68% <0.00%> (-6.39%)`	⬇️
cmd/cluster.go	`69.23% <0.00%> (-6.45%)`	⬇️
cmd/deployment.go	`68.82% <0.00%> (-4.19%)`	⬇️
cmd/workspace.go	`66.53% <0.00%> (+2.67%)`	⬆️
config/config.go	`47.05% <ø> (ø)`
cmd/version.go	`62.16% <36.36%> (-4.51%)`	⬇️
workspace/workspace.go	`74.57% <50.00%> (ø)`
config/context.go	`26.08% <60.00%> (ø)`
deployment/deployment.go	`87.00% <95.23%> (ø)`
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 965844e...8d3c9af. Read the comment docs.

adam2k · 2020-08-07T20:37:32Z

@paolaperaza can you give feedback on the upgrade message changes here?

andriisoldatenko · 2020-08-07T20:38:29Z

Coverage 43.40% 51.89% +8.48%

looks strange to me.

adam2k · 2020-08-07T20:57:17Z

Coverage 43.40% 51.89% +8.48%

looks strange to me.

I think it's a codecov error. I can rerun the tests and see if that resolves the difference.

paolaperaza · 2020-08-10T14:53:04Z

@adam2k Thanks for doing this! A few general comments:

UX looks good for users not authenticated to Astronomer
Should we list Server version in the way astro version also does? Seems like that'd be more consistent, enforce the 1:1 platform <> CLI minor version compatibility for users to see/be reminded of.
I wonder if we can push people to authenticate if they run this command but aren't... i.e.I'm thinking about a user that forgets to login, run astro uprade, updates to latest, then it turns out they're on an earlier platform version and get an error message about compatibility and have to downgrade. Including Server Version and a reminder to authenticate if that's not available might be enough?

In terms of copy, could simplify message to:

To upgrade to latest, run:... and then keep OR for homebrew users... What do you think?

adam2k · 2020-08-10T15:23:32Z

@adam2k Thanks for doing this! A few general comments:

* UX looks good for users not authenticated to Astronomer

* Should we list `Server version` in the way `astro version` also does? Seems like that'd be more consistent, enforce the 1:1 platform <> CLI minor version compatibility for users to see/be reminded of.

* I wonder if we can push people to authenticate if they run this command but aren't... i.e.I'm thinking about a user that forgets to login, run astro uprade, updates to latest, then it turns out they're on an earlier platform version and get an error message about compatibility and have to downgrade. Including `Server Version` and a reminder to authenticate if that's not available might be enough?

In terms of copy, could simplify message to:

To upgrade to latest, run:... and then keep OR for homebrew users... What do you think?

Thanks @paolaperaza! Yes, we can add in the server version here too. Do you think if we show the server version OR give the warning that they're not authenticated with a server, that would mitigate the last issue that you're describing enough? We might need to think about that experience some more and create a follow up issue because it might increase the scope of this issue a lot.

In the short term, do you think having some warning message like Upgrading beyond your server version may cause issues would be enough?

adam2k · 2020-08-10T15:57:24Z

@paolaperaza so this issue doesn't get lost. We discussed making some of the additional changes outside of the message change inside of this issue: https://github.com/astronomer/issues/issues/1636

paolaperaza · 2020-08-10T17:06:42Z

@adam2k Cool, commit looks good. Yea, I think if we list Server Version and whatever message is already there as-is that tells users to authenticate if they haven't already is enough to cover that problem/use case - definitely at least for now.

We can continue convo on the rest in astronomer/issues#1636!

andriisoldatenko · 2020-08-11T20:49:26Z

version/version.go

 	"io"

-	"github.com/pkg/errors"


pls undo this change, bad formating:
must be
STD lib
3rd party
custom pkgs

Oh, weird. My editor auto organized those. It looks like it's alphabetical from github. I'll try to revert.

I see what happened. I removed the empty space between "github.com/pkg/errors" and the astro imports and it grouped them altogether.

andriisoldatenko · 2020-08-11T20:50:52Z

version/version.go


 	return nil
 }

 // CheckForUpdate checks current version against latest on github
-func CheckForUpdate(client *github.Client, out io.Writer) error {
+func CheckForUpdate(client *houston.Client, ghClient *github.Client, out io.Writer) error {


JFYI for go code styling absolutely normal to rename ghClient to ghc.

version/version.go

andriisoldatenko · 2020-08-11T20:53:18Z

version/version.go

@@ -91,3 +84,17 @@ func isValidVersion(version string) bool {
 	}
 	return true
 }
+
+// printServerVersion outputs current server version
+func printServerVersion(client *houston.Client, out io.Writer) error {


why do we need to copy this?
DRY? for

astro-cli/version/version.go

Lines 33 to 40 in e7a3f35

appCfg, err := deployment.AppConfig(client)

if err != nil {

fmt.Fprintf(out, messages.HOUSTON_CURRENT_VERSION+"\n", "Please authenticate to a cluster to see server version")

}

if appCfg != nil {

fmt.Fprintf(out, messages.HOUSTON_CURRENT_VERSION+"\n", appCfg.Version)

}

Yeah, I was trying to keep it DRY. This method is now being invoked twice. Once on line 33 and once on 70.

version/version.go

Co-authored-by: Andrii Soldatenko <andrii.soldatenko@gmail.com>

adam2k · 2020-08-12T14:15:37Z

@andriisoldatenko do these updates look good to you now?

adam2k · 2020-08-25T00:39:55Z

I validated this on dev. The versions look off because it's expecting 0.19.0 to be the latest tag in GH (right now it's still 0.18.0). As soon as we take 0.19.0 out of prerelease this will all look correct.

Testing on dev cluster:

$ astro upgrade
A new minor version of Astro CLI is available. Your version is 0.18.0 and 0.19.0 is the latest.
See https://www.astronomer.io/docs/cli-quickstart for more information.
Astro CLI Version: v0.18.0 (2020.08.07)
Astro CLI Latest: v0.18.0  (2020.08.07)
You are running the latest version.

* Remove Houston version check on upgrade * Update message copy * Print server version for upgrade * Print the server version during upgrade * Apply suggestions from code review Co-authored-by: Andrii Soldatenko <andrii.soldatenko@gmail.com> Co-authored-by: Andrii Soldatenko <andrii.soldatenko@gmail.com>

Remove Houston version check on upgrade

83afd51

adam2k requested a review from paolaperaza August 7, 2020 20:36

adam2k self-assigned this Aug 7, 2020

adam2k marked this pull request as ready for review August 7, 2020 20:37

adam2k requested review from andriisoldatenko and schnie as code owners August 7, 2020 20:37

Update message copy

7838a4b

Adam Vandover added 2 commits August 10, 2020 13:33

Print server version for upgrade

b5526bb

Print the server version during upgrade

1c0e722

andriisoldatenko suggested changes Aug 11, 2020

View reviewed changes

andriisoldatenko reviewed Aug 11, 2020

View reviewed changes

version/version.go Outdated Show resolved Hide resolved

andriisoldatenko reviewed Aug 11, 2020

View reviewed changes

version/version.go Outdated Show resolved Hide resolved

andriisoldatenko suggested changes Aug 11, 2020

View reviewed changes

adam2k commented Aug 11, 2020

View reviewed changes

version/version.go Outdated Show resolved Hide resolved

Apply suggestions from code review

8d3c9af

Co-authored-by: Andrii Soldatenko <andrii.soldatenko@gmail.com>

adam2k force-pushed the fix/upgrade-version-check branch from bfc4c08 to 8d3c9af Compare August 11, 2020 22:44

schnie approved these changes Aug 13, 2020

View reviewed changes

andriisoldatenko self-requested a review August 13, 2020 17:39

andriisoldatenko approved these changes Aug 13, 2020

View reviewed changes

adam2k merged commit 8c8baec into main Aug 13, 2020

adam2k deleted the fix/upgrade-version-check branch August 13, 2020 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Houston version check on upgrade #353

Remove Houston version check on upgrade #353

adam2k commented Aug 7, 2020 •

edited

codecov bot commented Aug 7, 2020 •

edited

adam2k commented Aug 7, 2020

andriisoldatenko commented Aug 7, 2020 •

edited

adam2k commented Aug 7, 2020 •

edited

paolaperaza commented Aug 10, 2020

adam2k commented Aug 10, 2020

adam2k commented Aug 10, 2020

paolaperaza commented Aug 10, 2020

andriisoldatenko Aug 11, 2020

adam2k Aug 11, 2020

adam2k Aug 11, 2020

andriisoldatenko Aug 11, 2020

andriisoldatenko Aug 11, 2020

adam2k Aug 11, 2020 •

edited

adam2k commented Aug 12, 2020

adam2k commented Aug 25, 2020 •

edited

	appCfg, err := deployment.AppConfig(client)
	if err != nil {
	fmt.Fprintf(out, messages.HOUSTON_CURRENT_VERSION+"\n", "Please authenticate to a cluster to see server version")
	}

	if appCfg != nil {
	fmt.Fprintf(out, messages.HOUSTON_CURRENT_VERSION+"\n", appCfg.Version)
	}

Remove Houston version check on upgrade #353

Remove Houston version check on upgrade #353

Conversation

adam2k commented Aug 7, 2020 • edited

Description

🎟 Issue(s)

🧪 Functional Testing

📋 Checklist

codecov bot commented Aug 7, 2020 • edited

Codecov Report

adam2k commented Aug 7, 2020

andriisoldatenko commented Aug 7, 2020 • edited

adam2k commented Aug 7, 2020 • edited

paolaperaza commented Aug 10, 2020

adam2k commented Aug 10, 2020

adam2k commented Aug 10, 2020

paolaperaza commented Aug 10, 2020

andriisoldatenko Aug 11, 2020

Choose a reason for hiding this comment

adam2k Aug 11, 2020

Choose a reason for hiding this comment

adam2k Aug 11, 2020

Choose a reason for hiding this comment

andriisoldatenko Aug 11, 2020

Choose a reason for hiding this comment

andriisoldatenko Aug 11, 2020

Choose a reason for hiding this comment

adam2k Aug 11, 2020 • edited

Choose a reason for hiding this comment

adam2k commented Aug 12, 2020

adam2k commented Aug 25, 2020 • edited

adam2k commented Aug 7, 2020 •

edited

codecov bot commented Aug 7, 2020 •

edited

andriisoldatenko commented Aug 7, 2020 •

edited

adam2k commented Aug 7, 2020 •

edited

adam2k Aug 11, 2020 •

edited

adam2k commented Aug 25, 2020 •

edited